Re: [PATCH v1 1/1] gas: expr: fix support .long 0U and .long 0u

[Michael Eager <eager@eagercon.com>]

On 10/1/23 09:22, Frager, Neal wrote:
>>> Fix support for .long 0U and .long 0u in GCC.
>>>
>>> This patch has been tested for years of AMD Xilinx Yocto releases as
>>> part of the following patch set:
>>>
>>> https://github.com/Xilinx/meta-xilinx/tree/master/meta-microblaze/rec
>>> ipes-devtools/binutils/binutils
>>>
>>> Signed-off-by: Neal Frager <neal.frager@amd.com>
>>> ---
>>>    gas/expr.c | 9 +++++++++
>>>    1 file changed, 9 insertions(+)
>>
>> Without a testcase demonstrating what's wrong, I'd almost be inclined
>> to say that ...
>>
>>> --- a/gas/expr.c
>>> +++ b/gas/expr.c
>>> @@ -824,6 +824,15 @@ operand (expressionS *expressionP, enum expr_mode mode)
>>>    	      break;
>>>    	    }
>>>    	}
>>> +      if ((*input_line_pointer == 'U') || (*input_line_pointer == 'u'))
>>> +	{
>>> +	  input_line_pointer--;
>>> +
>>> +	  integer_constant ((NUMBERS_WITH_SUFFIX || flag_m68k_mri)
>>> +	                    ? 0 : 10,
>>> +	                    expressionP);
>>> +	  break;
>>> +	}
>>>          c = *input_line_pointer;
>>>          switch (c)
>>>    	{
>>
>> ... this ought to be covered by logic in integer_constant() already.
>> But I think I see what the issue is. Nevertheless, go look for
>> tc_allow_U_suffix, which wants using here as well. Further I think the
>> same issue then exists for L/l suffixes?
>>
>> Plus I think you want to add the new code to the switch() statement
>> rather than immediately ahead of it.
>>
>> Jan
>>
> 
> Hi Michael, Jan,
> 
>> Neal --
> 
>> Can you provide a test case for this patch?  I'm not able to reproduce any error.
> 
>> Please take a look at gas/expr.c:541-543 (PR 19910: Look for, and ignore, a U suffix to the number).  Is this a fix for the same issue?
>> https://sourceware.org/bugzilla/show_bug.cgi?id=19910
> 
>> As Jan says, it looks like this code, if needed, should be in the switch
>>   statement.
> 
> Thank you for your review.
> 
> I would just like to clarify where this patch is coming from.
> I am not actually the author of the patches I am submitting.
> When we provide a Microblaze toolchain with PetaLinux, Yocto or Vitis, we are actually applying all of the following patches to the binutils portion of the toolchain:
> https://github.com/Xilinx/meta-xilinx/tree/master/meta-microblaze/recipes-devtools/binutils/binutils
> 
> Instead of continuing to carry the maintenance of this patch set with all of our releases, I would like to get as many of these patches upstreamed as possible.
> 
> This particular patch is coming from here:
> https://github.com/Xilinx/meta-xilinx/blob/master/meta-microblaze/recipes-devtools/binutils/binutils/0009-Patch-Microblaze-fixed-bug-in-GCC-so-that-It-will-su.patch
> 
> It is possible that this patch is not required anymore.  I will check internally at AMD Xilinx to see if I can identify a way to reproduce the error that this patch is solving.
> 
> Until I can prove that this patch is still needed, let's skip it.  If it is really no longer necessary, I will try to get it removed from our list of patches.
> 
> Thanks for your help!
> 
> Best regards,
> Neal Frager
> AMD


Hi Neal --

In general, a patch needs to be for a confirmed bug, not hypothetical 
and not for a problem that has already been fixed.  It also needs to 
build with the top-of-tree sources cleanly.  If possible, there should 
be a test case which shows that there is a bug and that the patch fixes is.

When you sign off on a patch, that means that you are affirming that you 
have reviewed the patch and confirmed that this is the case.

This reduces the amount of effort that I and others have to expend to 
review patches and avoids adding unnecessary changes to the source.

-- 
Michael Eager