[PATCH] RISC-V: Move __global_pointer$ to .sdata

[PATCH] RISC-V: Move __global_pointer$ to .sdata

[Palmer Dabbelt]

We're putting __global_pointer$ in SHN_ABS right now, but glibc uses
PC-relative references to load up those symbols.  It turns out we allow
these references to resolve even in PIC, which is at best odd and
possibly just a bug.

Let's at least stop emitting these for now, we can try and figure out
how to stop resolving them later.

---

I haven't tested any of this at all, but it come up during the RISC-V
LLVM sync meeting today so I figured I'd send the patch to start a
discusion.  I'd very much bet this is broken...

Jim has a bug for the odd symbol placement
<https://sourceware.org/bugzilla/show_bug.cgi?id=24678>.  IIRC I also
filed a bug to stop allowing PC-relative references against SHN_ABS, but
I can't find it.
---
 ld/emulparams/elf32lriscv-defs.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/ld/emulparams/elf32lriscv-defs.sh b/ld/emulparams/elf32lriscv-defs.sh
index b823cedacab..186f2bf6e11 100644
--- a/ld/emulparams/elf32lriscv-defs.sh
+++ b/ld/emulparams/elf32lriscv-defs.sh
@@ -33,17 +33,17 @@ COMMONPAGESIZE="CONSTANT (COMMONPAGESIZE)"
 
 DATA_START_SYMBOLS="${CREATE_SHLIB-__DATA_BEGIN__ = .;}"
 
-SDATA_START_SYMBOLS="${CREATE_SHLIB-__SDATA_BEGIN__ = .;}
-    *(.srodata.cst16) *(.srodata.cst8) *(.srodata.cst4) *(.srodata.cst2) *(.srodata .srodata.*)"
-
-INITIAL_READONLY_SECTIONS=".interp         : { *(.interp) } ${CREATE_PIE-${INITIAL_READONLY_SECTIONS}}"
-INITIAL_READONLY_SECTIONS="${RELOCATING+${CREATE_SHLIB-${INITIAL_READONLY_SECTIONS}}}"
-
 # We must cover as much of sdata as possible if it exists.  If sdata+bss is
 # smaller than 0x1000 then we should start from bss end to cover as much of
 # the program as possible.  But we can't allow gp to cover any of rodata, as
 # the address of variables in rodata may change during relaxation, so we start
 # from data in that case.
-OTHER_END_SYMBOLS="${CREATE_SHLIB-__BSS_END__ = .;
+SDATA_START_SYMBOLS="${CREATE_SHLIB-__SDATA_BEGIN__ = .;}
+    *(.srodata.cst16) *(.srodata.cst8) *(.srodata.cst4) *(.srodata.cst2) *(.srodata .srodata.*)
     __global_pointer$ = MIN(__SDATA_BEGIN__ + 0x800,
 		            MAX(__DATA_BEGIN__ + 0x800, __BSS_END__ - 0x800));}"
+
+INITIAL_READONLY_SECTIONS=".interp         : { *(.interp) } ${CREATE_PIE-${INITIAL_READONLY_SECTIONS}}"
+INITIAL_READONLY_SECTIONS="${RELOCATING+${CREATE_SHLIB-${INITIAL_READONLY_SECTIONS}}}"
+
+OTHER_END_SYMBOLS="${CREATE_SHLIB-__BSS_END__ = .;"
-- 
2.40.1

Re: [PATCH] RISC-V: Move __global_pointer$ to .sdata

[Nelson Chu]

[-- Attachment #1: Type: text/plain, Size: 1670 bytes --]

>
> -SDATA_START_SYMBOLS="${CREATE_SHLIB-__SDATA_BEGIN__ = .;}
> -    *(.srodata.cst16) *(.srodata.cst8) *(.srodata.cst4) *(.srodata.cst2)
> *(.srodata .srodata.*)"
> -
> -INITIAL_READONLY_SECTIONS=".interp         : { *(.interp) }
> ${CREATE_PIE-${INITIAL_READONLY_SECTIONS}}"
>
> -INITIAL_READONLY_SECTIONS="${RELOCATING+${CREATE_SHLIB-${INITIAL_READONLY_SECTIONS}}}"
> -
>  # We must cover as much of sdata as possible if it exists.  If sdata+bss
> is
>  # smaller than 0x1000 then we should start from bss end to cover as much
> of
>  # the program as possible.  But we can't allow gp to cover any of rodata,
> as
>  # the address of variables in rodata may change during relaxation, so we
> start
>  # from data in that case.
> -OTHER_END_SYMBOLS="${CREATE_SHLIB-__BSS_END__ = .;
> +SDATA_START_SYMBOLS="${CREATE_SHLIB-__SDATA_BEGIN__ = .;}
> +    *(.srodata.cst16) *(.srodata.cst8) *(.srodata.cst4) *(.srodata.cst2)
> *(.srodata .srodata.*)
>      __global_pointer$ = MIN(__SDATA_BEGIN__ + 0x800,
>                             MAX(__DATA_BEGIN__ + 0x800, __BSS_END__ -
> 0x800));}"
> +
> +INITIAL_READONLY_SECTIONS=".interp         : { *(.interp) }
> ${CREATE_PIE-${INITIAL_READONLY_SECTIONS}}"
>
> +INITIAL_READONLY_SECTIONS="${RELOCATING+${CREATE_SHLIB-${INITIAL_READONLY_SECTIONS}}}"
> +
> +OTHER_END_SYMBOLS="${CREATE_SHLIB-__BSS_END__ = .;"
>

Just a minor case and not sure if it is right - If we don't have any sdata,
then the gp might be missing since it belongs to sdata?  So the gp value
will be regarded as zero internally, and it's just like we disable the gp
relaxation when there is no sdata, but it can be enabled before this change.

Thanks
Nelson

Re: [PATCH] RISC-V: Move __global_pointer$ to .sdata

[Nelson Chu]

[-- Attachment #1: Type: text/plain, Size: 5497 bytes --]

On Fri, Jun 9, 2023 at 8:40 AM Nelson Chu <nelson@rivosinc.com> wrote:

>
>
>> -SDATA_START_SYMBOLS="${CREATE_SHLIB-__SDATA_BEGIN__ = .;}
>> -    *(.srodata.cst16) *(.srodata.cst8) *(.srodata.cst4) *(.srodata.cst2)
>> *(.srodata .srodata.*)"
>> -
>> -INITIAL_READONLY_SECTIONS=".interp         : { *(.interp) }
>> ${CREATE_PIE-${INITIAL_READONLY_SECTIONS}}"
>>
>> -INITIAL_READONLY_SECTIONS="${RELOCATING+${CREATE_SHLIB-${INITIAL_READONLY_SECTIONS}}}"
>> -
>>  # We must cover as much of sdata as possible if it exists.  If sdata+bss
>> is
>>  # smaller than 0x1000 then we should start from bss end to cover as much
>> of
>>  # the program as possible.  But we can't allow gp to cover any of
>> rodata, as
>>  # the address of variables in rodata may change during relaxation, so we
>> start
>>  # from data in that case.
>> -OTHER_END_SYMBOLS="${CREATE_SHLIB-__BSS_END__ = .;
>> +SDATA_START_SYMBOLS="${CREATE_SHLIB-__SDATA_BEGIN__ = .;}
>> +    *(.srodata.cst16) *(.srodata.cst8) *(.srodata.cst4) *(.srodata.cst2)
>> *(.srodata .srodata.*)
>>      __global_pointer$ = MIN(__SDATA_BEGIN__ + 0x800,
>>                             MAX(__DATA_BEGIN__ + 0x800, __BSS_END__ -
>> 0x800));}"
>> +
>> +INITIAL_READONLY_SECTIONS=".interp         : { *(.interp) }
>> ${CREATE_PIE-${INITIAL_READONLY_SECTIONS}}"
>>
>> +INITIAL_READONLY_SECTIONS="${RELOCATING+${CREATE_SHLIB-${INITIAL_READONLY_SECTIONS}}}"
>> +
>> +OTHER_END_SYMBOLS="${CREATE_SHLIB-__BSS_END__ = .;"
>>
>
> Just a minor case and not sure if it is right - If we don't have any
> sdata, then the gp might be missing since it belongs to sdata?
>

Updated after experiments, if we don't have sdata, then the symbols defined
in there will be related to the existing previous output section.

Anyway, if we prefer to make the global pointer non-absolute, then there
are two options,
1. Similar to this patch, defined it into the output data or sdata section,
to make it related to data or sdata
2. Refer to Jim's solution in pr24678, define it outside the output
sections, but in the range of sbss and bss sections, then use "." to
represent __BSS_END__ to calculate the value, so that gp will still
belonged to bss section.

* Option 1 has a problem - the __BSS_END__ is defined outside the output
sections, and even if it is a related address, it will be regarded as a
number in the sdata or data section.  So the final gp will be wrong and
plus the current address ".".  So if we want to defined and let gp into and
related to sdata (or data), then the following change should work, but
looks  not pretty good,

--- a/ld/emulparams/elf32lriscv-defs.sh

+++ b/ld/emulparams/elf32lriscv-defs.sh

@@ -33,17 +33,12 @@ COMMONPAGESIZE="CONSTANT (COMMONPAGESIZE)"



 DATA_START_SYMBOLS="${CREATE_SHLIB-__DATA_BEGIN__ = .;}"



-SDATA_START_SYMBOLS="${CREATE_SHLIB-__SDATA_BEGIN__ = .;}

+SDATA_START_SYMBOLS="${CREATE_SHLIB-__SDATA_BEGIN__ = .;

+                   __global_pointer$ = MIN(__SDATA_BEGIN__ + 0x800,

+                                          MAX(__DATA_BEGIN__ + 0x800,
ABSOLUTE(__BSS_END__ - 0x800))) - ABSOLUTE(.);}

     *(.srodata.cst16) *(.srodata.cst8) *(.srodata.cst4) *(.srodata.cst2)
*(.srodata .srodata.*)"



 INITIAL_READONLY_SECTIONS=".interp         : { *(.interp) }
${CREATE_PIE-${INITIAL_READONLY_SECTIONS}}"


INITIAL_READONLY_SECTIONS="${RELOCATING+${CREATE_SHLIB-${INITIAL_READONLY_SECTIONS}}}"



-# We must cover as much of sdata as possible if it exists.  If sdata+bss is

-# smaller than 0x1000 then we should start from bss end to cover as much of

-# the program as possible.  But we can't allow gp to cover any of rodata,
as

-# the address of variables in rodata may change during relaxation, so we
start

-# from data in that case.

-OTHER_END_SYMBOLS="${CREATE_SHLIB-__BSS_END__ = .;

-    __global_pointer$ = MIN(__SDATA_BEGIN__ + 0x800,

-                           MAX(__DATA_BEGIN__ + 0x800, __BSS_END__ -
0x800));}"

+OTHER_END_SYMBOLS="${CREATE_SHLIB-__BSS_END__ = .;}"

* For option 2, copied from Jim's reply from pr24678,

--- a/ld/emulparams/elf32lriscv-defs.sh

+++ b/ld/emulparams/elf32lriscv-defs.sh

@@ -43,6 +43,6 @@
INITIAL_READONLY_SECTIONS="${RELOCATING+${CREATE_SHLIB-${INITIAL_READONLY_SECTIO

 # the program as possible.  But we can't allow gp to cover any of rodata,
as

 # the address of variables in rodata may change during relaxation, so we
start

 # from data in that case.

-OTHER_END_SYMBOLS="${CREATE_SHLIB-__BSS_END__ = .;

+OTHER_BSS_END_SYMBOLS="${CREATE_SHLIB-__BSS_END__ = .;

     __global_pointer$ = MIN(__SDATA_BEGIN__ + 0x800,

-                           MAX(__DATA_BEGIN__ + 0x800, __BSS_END__ -
0x800));}"

+                           MAX(__DATA_BEGIN__ + 0x800, . - 0x800));}"


Use current address "." to represent __BSS_END__ in the calculation, so
that the __global_pointer$ will always be related to the output bss
section.  Although I don't really understand why this works, since I
thought two related addresses of the different output sections calculated
by MAX should be an ABS rather than the related addresses of bss.  But
anyway, this change make gp always related to bss, no matter the value is
__SDATA_BEGIN__ + 0x800, __DATA_BEGIN__ + 0x800 or __BSS_END__ - 0x800.


Personally, I prefer option 2, but still cannot make the final choice, so
any thought is welcome.


Thanks

Nelson