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

[Jan Beulich <jbeulich@suse.com>]

On 01.10.2023 18: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.

Entirely fair. Yet then original authorship wants clarifying; in the
worst case to indicate that the original author cannot be determined
anymore.

Alternatively you could enter a bug (without reference to the original
work), which ought to trigger creation of a separate, newly written
patch (I've already added this to me todo list, just in case no v2
patch appears as per below). At which point you could then still drop
that patch at your end.

> 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.

I've double-checked that the change is still necessary. Please submit a
v2 (preferably, as Michael asked, with a testcase).

Jan