riscv: Stop considering R_RISCV_NONE as bad relocations

[PATCH] riscv: Stop considering R_RISCV_NONE as bad relocations

Posted by Alexandre Ghiti 2 months, 4 weeks ago

Even though those relocations should not be present in the final
vmlinux, there are a lot of them. And since those relocations are
considered "bad", they flood the compilation output which may hide some
legitimate bad relocations.

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
---
 arch/riscv/tools/relocs_check.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/tools/relocs_check.sh b/arch/riscv/tools/relocs_check.sh
index baeb2e7b2290558d696afbc5429d6a3c69ae49e1..742993e6a8cba72c657dd2f8f5dabc4c415e84bd 100755
--- a/arch/riscv/tools/relocs_check.sh
+++ b/arch/riscv/tools/relocs_check.sh
@@ -14,7 +14,9 @@ bad_relocs=$(
 ${srctree}/scripts/relocs_check.sh "$@" |
 	# These relocations are okay
 	#	R_RISCV_RELATIVE
-	grep -F -w -v 'R_RISCV_RELATIVE'
+	#	R_RISCV_NONE
+	grep -F -w -v 'R_RISCV_RELATIVE
+R_RISCV_NONE'
 )
 
 if [ -z "$bad_relocs" ]; then

---
base-commit: d7b8f8e20813f0179d8ef519541a3527e7661d3a
change-id: 20250710-dev-alex-riscv_none_bad_relocs_v1-ba6048681836

Best regards,
-- 
Alexandre Ghiti <alexghiti@rivosinc.com>

Re: [PATCH] riscv: Stop considering R_RISCV_NONE as bad relocations

Posted by Palmer Dabbelt 2 months, 4 weeks ago

On Thu, 10 Jul 2025 01:34:31 PDT (-0700), alexghiti@rivosinc.com wrote:
> Even though those relocations should not be present in the final
> vmlinux, there are a lot of them. And since those relocations are
> considered "bad", they flood the compilation output which may hide some
> legitimate bad relocations.
>
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
>  arch/riscv/tools/relocs_check.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/tools/relocs_check.sh b/arch/riscv/tools/relocs_check.sh
> index baeb2e7b2290558d696afbc5429d6a3c69ae49e1..742993e6a8cba72c657dd2f8f5dabc4c415e84bd 100755
> --- a/arch/riscv/tools/relocs_check.sh
> +++ b/arch/riscv/tools/relocs_check.sh
> @@ -14,7 +14,9 @@ bad_relocs=$(
>  ${srctree}/scripts/relocs_check.sh "$@" |
>  	# These relocations are okay
>  	#	R_RISCV_RELATIVE
> -	grep -F -w -v 'R_RISCV_RELATIVE'
> +	#	R_RISCV_NONE
> +	grep -F -w -v 'R_RISCV_RELATIVE
> +R_RISCV_NONE'
>  )

I'm not super opposed to it, but is there a way to just warn once or 
something?  It's probably best to still report something, as there's 
likely some sort of toolchain issue here.

Also: if you can reproduce it, Nelson can probably fix it.  I'm CCing 
him.

>
>  if [ -z "$bad_relocs" ]; then

Re: [PATCH] riscv: Stop considering R_RISCV_NONE as bad relocations

Posted by Jingwei Wang 2 months, 3 weeks ago

Hi all,

On Thu, Jul 10, 2025 at 11:43:00AM -0700, Palmer Dabbelt wrote:
> On Thu, 10 Jul 2025 01:34:31 PDT (-0700), alexghiti@rivosinc.com wrote:
> > Even though those relocations should not be present in the final
> > vmlinux, there are a lot of them. And since those relocations are
> > considered "bad", they flood the compilation output which may hide some
> > legitimate bad relocations.
> >
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > ---
> >  arch/riscv/tools/relocs_check.sh | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/tools/relocs_check.sh b/arch/riscv/tools/relocs_check.sh
> > index baeb2e7b2290558d696afbc5429d6a3c69ae49e1..742993e6a8cba72c657dd2f8f5dabc4c415e84bd 100755
> > --- a/arch/riscv/tools/relocs_check.sh
> > +++ b/arch/riscv/tools/relocs_check.sh
> > @@ -14,7 +14,9 @@ bad_relocs=$(
> >  ${srctree}/scripts/relocs_check.sh "$@" |
> >  	# These relocations are okay
> >  	#	R_RISCV_RELATIVE
> > -	grep -F -w -v 'R_RISCV_RELATIVE'
> > +	#	R_RISCV_NONE
> > +	grep -F -w -v 'R_RISCV_RELATIVE
> > +R_RISCV_NONE'
> >  )
>
> I'm not super opposed to it, but is there a way to just warn once or
> something?  It's probably best to still report something, as there's likely
> some sort of toolchain issue here.
>

I think Alexandre's approach is ideal from the kernel's perspective.
This doesn't really seem to be a bug; I see it more as a case where the
toolchain's handling of R_RISCV_NONE doesn't quite match the kernel's
expectations.

I found the large number of R_RISCV_NONE relocs only appear in the final
vmlinux. The key difference is the kernel build's --emit-relocs flag
and the *(.rela.text*) directive in vmlinux.lds.S. This combination
forces all relocation entries, including those marked as R_RISCV_NONE,
to be written verbatim into the final vmlinux.

I traced this back to BFD's implementation and found that during
relaxation (e.g., when an auipc+jalr is optimized to a jal), the linker
modifies the first reloc slot to R_RISCV_JAL and marks the second,
now-useless slot as R_RISCV_DELETE. In the cleanup phase, to prevent
reprocessing, BFD then changes the cleaned-up DELETE marker to
R_RISCV_NONE. This is how, when the kernel specifies --emit-relocs,
these R_RISCV_NONE markers get preserved in the final .rela.text section.

To truly change this, it seems to depend on whether the binutils
is willing to add a stage to clean up these harmless but
useless markers.

If possible, I was thinking we could perhaps iterate and remove the
R_RISCV_NONE entries from .rela.text before the alignment pass.

But if there's no agreement on the BFD side, Alexandre's approach still
seems correct and aligns with the psABI, where R_RISCV_NONE has no
operational meaning.

> Also: if you can reproduce it, Nelson can probably fix it.  I'm CCing him.
>

Reproducing the issue is simple: you just need a call instruction to
create a relaxation opportunity, then link with --emit-relocs and a
linker script that includes *(.rela.text*). :)

For convenience, I've put a minimal, self-contained reproduction case
here: https://gist.github.com/Jingwiw/762606e1dc3b77c352b394e8c5e846de

> >
> >  if [ -z "$bad_relocs" ]; then
>

Reviewed-by: Jingwei Wang <wangjingwei@iscas.ac.cn>

Thanks,
Jingwei

Re: [PATCH] riscv: Stop considering R_RISCV_NONE as bad relocations

Posted by Palmer Dabbelt 2 months, 3 weeks ago

On Fri, 11 Jul 2025 06:49:09 PDT (-0700), wangjingwei@iscas.ac.cn wrote:
> Hi all,
>
> On Thu, Jul 10, 2025 at 11:43:00AM -0700, Palmer Dabbelt wrote:
>> On Thu, 10 Jul 2025 01:34:31 PDT (-0700), alexghiti@rivosinc.com wrote:
>> > Even though those relocations should not be present in the final
>> > vmlinux, there are a lot of them. And since those relocations are
>> > considered "bad", they flood the compilation output which may hide some
>> > legitimate bad relocations.
>> >
>> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
>> > ---
>> >  arch/riscv/tools/relocs_check.sh | 4 +++-
>> >  1 file changed, 3 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/riscv/tools/relocs_check.sh b/arch/riscv/tools/relocs_check.sh
>> > index baeb2e7b2290558d696afbc5429d6a3c69ae49e1..742993e6a8cba72c657dd2f8f5dabc4c415e84bd 100755
>> > --- a/arch/riscv/tools/relocs_check.sh
>> > +++ b/arch/riscv/tools/relocs_check.sh
>> > @@ -14,7 +14,9 @@ bad_relocs=$(
>> >  ${srctree}/scripts/relocs_check.sh "$@" |
>> >  	# These relocations are okay
>> >  	#	R_RISCV_RELATIVE
>> > -	grep -F -w -v 'R_RISCV_RELATIVE'
>> > +	#	R_RISCV_NONE
>> > +	grep -F -w -v 'R_RISCV_RELATIVE
>> > +R_RISCV_NONE'
>> >  )
>>
>> I'm not super opposed to it, but is there a way to just warn once or
>> something?  It's probably best to still report something, as there's likely
>> some sort of toolchain issue here.
>>
>
> I think Alexandre's approach is ideal from the kernel's perspective.
> This doesn't really seem to be a bug; I see it more as a case where the
> toolchain's handling of R_RISCV_NONE doesn't quite match the kernel's
> expectations.
>
> I found the large number of R_RISCV_NONE relocs only appear in the final
> vmlinux. The key difference is the kernel build's --emit-relocs flag
> and the *(.rela.text*) directive in vmlinux.lds.S. This combination
> forces all relocation entries, including those marked as R_RISCV_NONE,
> to be written verbatim into the final vmlinux.

Ah, OK, if it's coming from "--emit-relocs" then actually it seems fine.  
So I think this is the right way to go, then.  It's on fixes, should 
show up for Linus later this week.

> I traced this back to BFD's implementation and found that during
> relaxation (e.g., when an auipc+jalr is optimized to a jal), the linker
> modifies the first reloc slot to R_RISCV_JAL and marks the second,
> now-useless slot as R_RISCV_DELETE. In the cleanup phase, to prevent
> reprocessing, BFD then changes the cleaned-up DELETE marker to
> R_RISCV_NONE. This is how, when the kernel specifies --emit-relocs,
> these R_RISCV_NONE markers get preserved in the final .rela.text section.
>
> To truly change this, it seems to depend on whether the binutils
> is willing to add a stage to clean up these harmless but
> useless markers.
>
> If possible, I was thinking we could perhaps iterate and remove the
> R_RISCV_NONE entries from .rela.text before the alignment pass.
>
> But if there's no agreement on the BFD side, Alexandre's approach still
> seems correct and aligns with the psABI, where R_RISCV_NONE has no
> operational meaning.
>
>> Also: if you can reproduce it, Nelson can probably fix it.  I'm CCing him.
>>
>
> Reproducing the issue is simple: you just need a call instruction to
> create a relaxation opportunity, then link with --emit-relocs and a
> linker script that includes *(.rela.text*). :)
>
> For convenience, I've put a minimal, self-contained reproduction case
> here: https://gist.github.com/Jingwiw/762606e1dc3b77c352b394e8c5e846de
>
>> >
>> >  if [ -z "$bad_relocs" ]; then
>>
>
> Reviewed-by: Jingwei Wang <wangjingwei@iscas.ac.cn>
>
> Thanks,
> Jingwei

Re: [PATCH] riscv: Stop considering R_RISCV_NONE as bad relocations

Posted by Ron Economos 2 months, 4 weeks ago

On 7/10/25 01:34, Alexandre Ghiti wrote:
> Even though those relocations should not be present in the final
> vmlinux, there are a lot of them. And since those relocations are
> considered "bad", they flood the compilation output which may hide some
> legitimate bad relocations.
>
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
>   arch/riscv/tools/relocs_check.sh | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/tools/relocs_check.sh b/arch/riscv/tools/relocs_check.sh
> index baeb2e7b2290558d696afbc5429d6a3c69ae49e1..742993e6a8cba72c657dd2f8f5dabc4c415e84bd 100755
> --- a/arch/riscv/tools/relocs_check.sh
> +++ b/arch/riscv/tools/relocs_check.sh
> @@ -14,7 +14,9 @@ bad_relocs=$(
>   ${srctree}/scripts/relocs_check.sh "$@" |
>   	# These relocations are okay
>   	#	R_RISCV_RELATIVE
> -	grep -F -w -v 'R_RISCV_RELATIVE'
> +	#	R_RISCV_NONE
> +	grep -F -w -v 'R_RISCV_RELATIVE
> +R_RISCV_NONE'
>   )
>   
>   if [ -z "$bad_relocs" ]; then
>
> ---
> base-commit: d7b8f8e20813f0179d8ef519541a3527e7661d3a
> change-id: 20250710-dev-alex-riscv_none_bad_relocs_v1-ba6048681836
>
> Best regards,

Works good.

Tested-by: Ron Economos <re@w6rz.net>