* [PATCH] RISC-V: Fix "withand" in LEB128 error messages
@ 2023-12-06 17:52 Palmer Dabbelt
2023-12-06 18:03 ` Fangrui Song
2023-12-07 1:28 ` Nelson Chu
0 siblings, 2 replies; 6+ messages in thread
From: Palmer Dabbelt @ 2023-12-06 17:52 UTC (permalink / raw)
To: binutils; +Cc: Palmer Dabbelt, David Abdurachmanov
This was split over multiple lines and ended up missing a space.
Reported-by: David Abdurachmanov <davidlt@rivosinc.com>
Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
---
bfd/elfnn-riscv.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index 5c4bf4bc3cb..042266e791b 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -2521,7 +2521,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
else
{
msg = ("Mismatched R_RISCV_SET_ULEB128, it must be paired with"
- "and applied before R_RISCV_SUB_ULEB128");
+ " and applied before R_RISCV_SUB_ULEB128");
r = bfd_reloc_dangerous;
}
break;
@@ -2537,7 +2537,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
else
{
msg = ("Mismatched R_RISCV_SUB_ULEB128, it must be paired with"
- "and applied after R_RISCV_SET_ULEB128");
+ " and applied after R_RISCV_SET_ULEB128");
r = bfd_reloc_dangerous;
}
break;
--
2.42.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] RISC-V: Fix "withand" in LEB128 error messages
2023-12-06 17:52 [PATCH] RISC-V: Fix "withand" in LEB128 error messages Palmer Dabbelt
@ 2023-12-06 18:03 ` Fangrui Song
2023-12-06 18:15 ` Palmer Dabbelt
2023-12-07 1:28 ` Nelson Chu
1 sibling, 1 reply; 6+ messages in thread
From: Fangrui Song @ 2023-12-06 18:03 UTC (permalink / raw)
To: Palmer Dabbelt; +Cc: binutils, David Abdurachmanov
On Wed, Dec 6, 2023 at 9:52 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> This was split over multiple lines and ended up missing a space.
>
> Reported-by: David Abdurachmanov <davidlt@rivosinc.com>
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> ---
> bfd/elfnn-riscv.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index 5c4bf4bc3cb..042266e791b 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -2521,7 +2521,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
> else
> {
> msg = ("Mismatched R_RISCV_SET_ULEB128, it must be paired with"
> - "and applied before R_RISCV_SUB_ULEB128");
> + " and applied before R_RISCV_SUB_ULEB128");
> r = bfd_reloc_dangerous;
> }
> break;
> @@ -2537,7 +2537,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
> else
> {
> msg = ("Mismatched R_RISCV_SUB_ULEB128, it must be paired with"
> - "and applied after R_RISCV_SET_ULEB128");
> + " and applied after R_RISCV_SET_ULEB128");
> r = bfd_reloc_dangerous;
> }
> break;
> --
> 2.42.1
>
LGTM. Ideally this error message should be tested using `#error:`
.reloc directive can be used to create a relocation.
.reloc ., R_RISCV_SET_ULEB128, w2
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] RISC-V: Fix "withand" in LEB128 error messages
2023-12-06 18:03 ` Fangrui Song
@ 2023-12-06 18:15 ` Palmer Dabbelt
2023-12-06 18:24 ` Fangrui Song
0 siblings, 1 reply; 6+ messages in thread
From: Palmer Dabbelt @ 2023-12-06 18:15 UTC (permalink / raw)
To: i; +Cc: binutils, davidlt
On Wed, 06 Dec 2023 10:03:43 PST (-0800), i@maskray.me wrote:
> On Wed, Dec 6, 2023 at 9:52 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>>
>> This was split over multiple lines and ended up missing a space.
>>
>> Reported-by: David Abdurachmanov <davidlt@rivosinc.com>
>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>> ---
>> bfd/elfnn-riscv.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
>> index 5c4bf4bc3cb..042266e791b 100644
>> --- a/bfd/elfnn-riscv.c
>> +++ b/bfd/elfnn-riscv.c
>> @@ -2521,7 +2521,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
>> else
>> {
>> msg = ("Mismatched R_RISCV_SET_ULEB128, it must be paired with"
>> - "and applied before R_RISCV_SUB_ULEB128");
>> + " and applied before R_RISCV_SUB_ULEB128");
>> r = bfd_reloc_dangerous;
>> }
>> break;
>> @@ -2537,7 +2537,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
>> else
>> {
>> msg = ("Mismatched R_RISCV_SUB_ULEB128, it must be paired with"
>> - "and applied after R_RISCV_SET_ULEB128");
>> + " and applied after R_RISCV_SET_ULEB128");
>> r = bfd_reloc_dangerous;
>> }
>> break;
>> --
>> 2.42.1
>>
>
> LGTM. Ideally this error message should be tested using `#error:`
> .reloc directive can be used to create a relocation.
>
> .reloc ., R_RISCV_SET_ULEB128, w2
Ya, seems reasonable. IIUC there's still some discussion in psABI land
as to exactly what the required semantics of these are, though, so maybe
we hold off on writing tests until things settle down?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] RISC-V: Fix "withand" in LEB128 error messages
2023-12-06 18:15 ` Palmer Dabbelt
@ 2023-12-06 18:24 ` Fangrui Song
0 siblings, 0 replies; 6+ messages in thread
From: Fangrui Song @ 2023-12-06 18:24 UTC (permalink / raw)
To: Palmer Dabbelt; +Cc: binutils, davidlt
On Wed, Dec 6, 2023 at 10:15 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> On Wed, 06 Dec 2023 10:03:43 PST (-0800), i@maskray.me wrote:
> > On Wed, Dec 6, 2023 at 9:52 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
> >>
> >> This was split over multiple lines and ended up missing a space.
> >>
> >> Reported-by: David Abdurachmanov <davidlt@rivosinc.com>
> >> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> >> ---
> >> bfd/elfnn-riscv.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> >> index 5c4bf4bc3cb..042266e791b 100644
> >> --- a/bfd/elfnn-riscv.c
> >> +++ b/bfd/elfnn-riscv.c
> >> @@ -2521,7 +2521,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
> >> else
> >> {
> >> msg = ("Mismatched R_RISCV_SET_ULEB128, it must be paired with"
> >> - "and applied before R_RISCV_SUB_ULEB128");
> >> + " and applied before R_RISCV_SUB_ULEB128");
> >> r = bfd_reloc_dangerous;
> >> }
> >> break;
> >> @@ -2537,7 +2537,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
> >> else
> >> {
> >> msg = ("Mismatched R_RISCV_SUB_ULEB128, it must be paired with"
> >> - "and applied after R_RISCV_SET_ULEB128");
> >> + " and applied after R_RISCV_SET_ULEB128");
> >> r = bfd_reloc_dangerous;
> >> }
> >> break;
> >> --
> >> 2.42.1
> >>
> >
> > LGTM. Ideally this error message should be tested using `#error:`
> > .reloc directive can be used to create a relocation.
> >
> > .reloc ., R_RISCV_SET_ULEB128, w2
>
> Ya, seems reasonable. IIUC there's still some discussion in psABI land
> as to exactly what the required semantics of these are, though, so maybe
> we hold off on writing tests until things settle down?
Yes I am aware of
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/413 whose
justification isn't clear.
---
In llvm-project, a somewhat common practice is to pre-commit test
improvement, and when a change arises, make a commit including both
the functional change and the test updates.
This makes it clearer how an individual commit changes the behavior
and sometimes makes a large patch smaller.
Of course in some cases this practice adds some complexity and may not
be worth doing.
I think the behavior of R_RISCV_SET_ULEB128 is worth checking. The
test for this error message seems overdue.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] RISC-V: Fix "withand" in LEB128 error messages
2023-12-06 17:52 [PATCH] RISC-V: Fix "withand" in LEB128 error messages Palmer Dabbelt
2023-12-06 18:03 ` Fangrui Song
@ 2023-12-07 1:28 ` Nelson Chu
1 sibling, 0 replies; 6+ messages in thread
From: Nelson Chu @ 2023-12-07 1:28 UTC (permalink / raw)
To: Palmer Dabbelt; +Cc: binutils, David Abdurachmanov, Fangrui Song
[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]
Just inform that this patch is committed. For other discussion about
uleb128 overflow checking, we can move to another reply thread and psabi.
Nelson
On Thu, Dec 7, 2023 at 1:53 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
> This was split over multiple lines and ended up missing a space.
>
> Reported-by: David Abdurachmanov <davidlt@rivosinc.com>
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> ---
> bfd/elfnn-riscv.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index 5c4bf4bc3cb..042266e791b 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -2521,7 +2521,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
> else
> {
> msg = ("Mismatched R_RISCV_SET_ULEB128, it must be paired
> with"
> - "and applied before R_RISCV_SUB_ULEB128");
> + " and applied before R_RISCV_SUB_ULEB128");
> r = bfd_reloc_dangerous;
> }
> break;
> @@ -2537,7 +2537,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
> else
> {
> msg = ("Mismatched R_RISCV_SUB_ULEB128, it must be paired
> with"
> - "and applied after R_RISCV_SET_ULEB128");
> + " and applied after R_RISCV_SET_ULEB128");
> r = bfd_reloc_dangerous;
> }
> break;
> --
> 2.42.1
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] RISC-V: Fix "withand" in LEB128 error messages
[not found] <DS7PR12MB576581F139B7D8895C7C3CB7CB84A@DS7PR12MB5765.namprd12.prod.outlook.com>
@ 2023-12-06 19:07 ` Palmer Dabbelt
0 siblings, 0 replies; 6+ messages in thread
From: Palmer Dabbelt @ 2023-12-06 19:07 UTC (permalink / raw)
To: i, charlie; +Cc: binutils, davidlt
On Wed, 06 Dec 2023 10:24:46 PST (-0800), i@maskray.me wrote:
> On Wed, Dec 6, 2023 at 10:15 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>>
>> On Wed, 06 Dec 2023 10:03:43 PST (-0800), i@maskray.me wrote:
>> > On Wed, Dec 6, 2023 at 9:52 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>> >>
>> >> This was split over multiple lines and ended up missing a space.
>> >>
>> >> Reported-by: David Abdurachmanov <davidlt@rivosinc.com>
>> >> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>> >> ---
>> >> bfd/elfnn-riscv.c | 4 ++--
>> >> 1 file changed, 2 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
>> >> index 5c4bf4bc3cb..042266e791b 100644
>> >> --- a/bfd/elfnn-riscv.c
>> >> +++ b/bfd/elfnn-riscv.c
>> >> @@ -2521,7 +2521,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
>> >> else
>> >> {
>> >> msg = ("Mismatched R_RISCV_SET_ULEB128, it must be paired with"
>> >> - "and applied before R_RISCV_SUB_ULEB128");
>> >> + " and applied before R_RISCV_SUB_ULEB128");
>> >> r = bfd_reloc_dangerous;
>> >> }
>> >> break;
>> >> @@ -2537,7 +2537,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
>> >> else
>> >> {
>> >> msg = ("Mismatched R_RISCV_SUB_ULEB128, it must be paired with"
>> >> - "and applied after R_RISCV_SET_ULEB128");
>> >> + " and applied after R_RISCV_SET_ULEB128");
>> >> r = bfd_reloc_dangerous;
>> >> }
>> >> break;
>> >> --
>> >> 2.42.1
>> >>
>> >
>> > LGTM. Ideally this error message should be tested using `#error:`
>> > .reloc directive can be used to create a relocation.
>> >
>> > .reloc ., R_RISCV_SET_ULEB128, w2
>>
>> Ya, seems reasonable. IIUC there's still some discussion in psABI land
>> as to exactly what the required semantics of these are, though, so maybe
>> we hold off on writing tests until things settle down?
>
> Yes I am aware of
> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/413 whose
> justification isn't clear.
Ya, I don't think anyone over here is really clear on what we're trying
to do with this one. +Charlie, as we'd talked about it yesterday: I
don't think either of us care that much one way or the other, there's
just been some discussions that point both ways. I figured the best way
to sort it out would be to just start the psABI discussion.
>
> ---
>
> In llvm-project, a somewhat common practice is to pre-commit test
> improvement, and when a change arises, make a commit including both
> the functional change and the test updates.
> This makes it clearer how an individual commit changes the behavior
> and sometimes makes a large patch smaller.
>
> Of course in some cases this practice adds some complexity and may not
> be worth doing.
IMO we're somewhat behind on stuff like testing (and just general patch
hygine) in GNU land, but that sort of stuff isn't really up to me -- or
at least globally, I guess I could choose not to be lazy ;)
> I think the behavior of R_RISCV_SET_ULEB128 is worth checking. The
> test for this error message seems overdue.
Ya, seems reasonable. So maybe this psABI discussion is going to
resolve quickly, if the result is that we need to change how binutils
works then we'll need way more than this, so we can just deal with
testing then.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-07 1:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-06 17:52 [PATCH] RISC-V: Fix "withand" in LEB128 error messages Palmer Dabbelt
2023-12-06 18:03 ` Fangrui Song
2023-12-06 18:15 ` Palmer Dabbelt
2023-12-06 18:24 ` Fangrui Song
2023-12-07 1:28 ` Nelson Chu
[not found] <DS7PR12MB576581F139B7D8895C7C3CB7CB84A@DS7PR12MB5765.namprd12.prod.outlook.com>
2023-12-06 19:07 ` Palmer Dabbelt
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).