* [PATCH v1 1/1] gas: expr: fix support .long 0U and .long 0u @ 2023-09-27 13:21 Neal Frager 2023-09-27 13:33 ` Jan Beulich 0 siblings, 1 reply; 7+ messages in thread From: Neal Frager @ 2023-09-27 13:21 UTC (permalink / raw) To: binutils Cc: ibai.erkiaga-elorza, nagaraju.mekala, mark.hatle, sadanand.mutyala, appa.rao.nali, vidhumouli.hunsigida, luca.ceresoli, 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/1] gas: expr: fix support .long 0U and .long 0u 2023-09-27 13:21 [PATCH v1 1/1] gas: expr: fix support .long 0U and .long 0u Neal Frager @ 2023-09-27 13:33 ` Jan Beulich 2023-09-28 19:27 ` Michael Eager 0 siblings, 1 reply; 7+ messages in thread From: Jan Beulich @ 2023-09-27 13:33 UTC (permalink / raw) To: Neal Frager Cc: ibai.erkiaga-elorza, nagaraju.mekala, mark.hatle, sadanand.mutyala, appa.rao.nali, vidhumouli.hunsigida, luca.ceresoli, binutils 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/1] gas: expr: fix support .long 0U and .long 0u 2023-09-27 13:33 ` Jan Beulich @ 2023-09-28 19:27 ` Michael Eager 2023-10-01 16:22 ` Frager, Neal 0 siblings, 1 reply; 7+ messages in thread From: Michael Eager @ 2023-09-28 19:27 UTC (permalink / raw) To: Jan Beulich, Neal Frager Cc: ibai.erkiaga-elorza, nagaraju.mekala, mark.hatle, sadanand.mutyala, appa.rao.nali, vidhumouli.hunsigida, luca.ceresoli, binutils 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v1 1/1] gas: expr: fix support .long 0U and .long 0u 2023-09-28 19:27 ` Michael Eager @ 2023-10-01 16:22 ` Frager, Neal 2023-10-01 16:34 ` Michael Eager 2023-10-16 9:18 ` Jan Beulich 0 siblings, 2 replies; 7+ messages in thread From: Frager, Neal @ 2023-10-01 16:22 UTC (permalink / raw) To: Michael Eager, Jan Beulich Cc: Erkiaga Elorza, Ibai, Mekala, Nagaraju, Hatle, Mark, Mutyala, Sadanand, Nali, Appa Rao, Hunsigida, Vidhumouli, luca.ceresoli, binutils >> 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/1] gas: expr: fix support .long 0U and .long 0u 2023-10-01 16:22 ` Frager, Neal @ 2023-10-01 16:34 ` Michael Eager 2023-10-01 18:06 ` Frager, Neal 2023-10-16 9:18 ` Jan Beulich 1 sibling, 1 reply; 7+ messages in thread From: Michael Eager @ 2023-10-01 16:34 UTC (permalink / raw) To: Frager, Neal, Jan Beulich Cc: Erkiaga Elorza, Ibai, Mekala, Nagaraju, Hatle, Mark, Mutyala, Sadanand, Nali, Appa Rao, Hunsigida, Vidhumouli, luca.ceresoli, binutils 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v1 1/1] gas: expr: fix support .long 0U and .long 0u 2023-10-01 16:34 ` Michael Eager @ 2023-10-01 18:06 ` Frager, Neal 0 siblings, 0 replies; 7+ messages in thread From: Frager, Neal @ 2023-10-01 18:06 UTC (permalink / raw) To: Michael Eager, Jan Beulich Cc: Erkiaga Elorza, Ibai, Mekala, Nagaraju, Hatle, Mark, Mutyala, Sadanand, Nali, Appa Rao, Hunsigida, Vidhumouli, luca.ceresoli, binutils >>> 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/1] gas: expr: fix support .long 0U and .long 0u 2023-10-01 16:22 ` Frager, Neal 2023-10-01 16:34 ` Michael Eager @ 2023-10-16 9:18 ` Jan Beulich 1 sibling, 0 replies; 7+ messages in thread From: Jan Beulich @ 2023-10-16 9:18 UTC (permalink / raw) To: Frager, Neal Cc: Erkiaga Elorza, Ibai, Mekala, Nagaraju, Hatle, Mark, Mutyala, Sadanand, Nali, Appa Rao, Hunsigida, Vidhumouli, luca.ceresoli, binutils, Michael Eager 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-10-16 9:18 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-09-27 13:21 [PATCH v1 1/1] gas: expr: fix support .long 0U and .long 0u Neal Frager 2023-09-27 13:33 ` Jan Beulich 2023-09-28 19:27 ` Michael Eager 2023-10-01 16:22 ` Frager, Neal 2023-10-01 16:34 ` Michael Eager 2023-10-01 18:06 ` Frager, Neal 2023-10-16 9:18 ` Jan Beulich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).