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

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

[Neal Frager]

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/recipes-devtools/binutils/binutils

Signed-off-by: Neal Frager <neal.frager@amd.com>
---
 gas/expr.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gas/expr.c b/gas/expr.c
index 204cfebc4c7..0d4e955b63a 100644
--- 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)
 	{
-- 
2.25.1

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

[Jan Beulich]

On 27.09.2023 15:21, Neal Frager via Binutils 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/recipes-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

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

[Michael Eager]

On 9/27/23 06:33, Jan Beulich via Binutils wrote:
> On 27.09.2023 15:21, Neal Frager via Binutils 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/recipes-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
> 

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.


-- 
Michael Eager

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

[Frager, Neal]

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

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

[Michael Eager]

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

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

[Frager, Neal]

>>> 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/re
>>> c
>>> 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/reci
> pes-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/reci
> pes-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

Hi Michael,

I completely understand.  I will make sure to only upstream the patches that I am sure we need and why we need them.

Thank you for your support.

Best regards,
Neal Frager
AMD

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

[Jan Beulich]

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