diff mbox series

RISC-V: Fix "withand" in LEB128 error messages

Message ID 20231206175231.4384-1-palmer@rivosinc.com
State Unresolved
Headers
Series RISC-V: Fix "withand" in LEB128 error messages |

Checks

Context Check Description
snail/binutils-gdb-check warning Git am fail log

Commit Message

Palmer Dabbelt Dec. 6, 2023, 5:52 p.m. UTC 
  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(-)

Comments

Fangrui Song Dec. 6, 2023, 6:03 p.m. UTC | #1 
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
Palmer Dabbelt Dec. 6, 2023, 6:15 p.m. UTC | #2 
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?
Fangrui Song Dec. 6, 2023, 6:24 p.m. UTC | #3 
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.
Nelson Chu Dec. 7, 2023, 1:28 a.m. UTC | #4 
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
>
>
diff mbox series

Patch

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;