diff mbox series

elf: work around a gcc bug in elf_get_dynamic_info

Message ID 20210111151146.27850-1-szabolcs.nagy@arm.com
State Superseded
Headers
Series elf: work around a gcc bug in elf_get_dynamic_info |

Commit Message

Szabolcs Nagy Jan. 11, 2021, 3:11 p.m. UTC 
  Since commit 2f056e8a5dd4dc0f075413f931e82cede37d1057
"aarch64: define PI_STATIC_AND_HIDDEN",
building glibc with gcc-8 on aarch64 fails with

/BLD/elf/librtld.os: in function `elf_get_dynamic_info':
/SRC/elf/get-dynamic-info.h:70:(.text+0xad8): relocation truncated to
 fit: R_AARCH64_ADR_PREL_PG_HI21 against symbol `_rtld_local' defined
 in .data section in /BLD/elf/librtld.os

This is a gcc bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98618

Rewriting the affected code in elf_get_dynamic_info
seems to make this issue go away on old gcc.

The change makes the logic a bit clearer too (by
separating the index computation and array update)
and drops an older gcc workaround (since gcc 4.6
is no longer supported).
---
note: with new gcc the code generation only changes
slightly on aarch64 (the code becomes a bit smaller),
with old gcc the change is not a robust fix for the
issue but it seems to be be sufficient in my testing.

the change only expected to have minor effect on
other targets, since it's mostly cosmetic.

 elf/get-dynamic-info.h | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

Comments

Adhemerval Zanella Netto Jan. 12, 2021, 5:52 p.m. UTC | #1 
On 11/01/2021 12:11, Szabolcs Nagy via Libc-alpha wrote:
> Since commit 2f056e8a5dd4dc0f075413f931e82cede37d1057
> "aarch64: define PI_STATIC_AND_HIDDEN",
> building glibc with gcc-8 on aarch64 fails with
> 
> /BLD/elf/librtld.os: in function `elf_get_dynamic_info':
> /SRC/elf/get-dynamic-info.h:70:(.text+0xad8): relocation truncated to
>  fit: R_AARCH64_ADR_PREL_PG_HI21 against symbol `_rtld_local' defined
>  in .data section in /BLD/elf/librtld.os
> 
> This is a gcc bug:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98618
> 
> Rewriting the affected code in elf_get_dynamic_info
> seems to make this issue go away on old gcc.
> 
> The change makes the logic a bit clearer too (by
> separating the index computation and array update)
> and drops an older gcc workaround (since gcc 4.6
> is no longer supported).

So if I understood correctly this bug affects only gcc-8? Or
does it affect older version as well? From comment #3, Wilco stated
it was fixed by on gcc-10, so I take it was backported to gcc-9 
already (since you didn't see it on gcc-9).

I did a sniff build tests with different gcc (8 and 9 mostly) on 
different architectures and it seems ok.

LGTM with the just nit below about VERSYMIDX.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
> note: with new gcc the code generation only changes
> slightly on aarch64 (the code becomes a bit smaller),
> with old gcc the change is not a robust fix for the
> issue but it seems to be be sufficient in my testing.
> 
> the change only expected to have minor effect on
> other targets, since it's mostly cosmetic.
> 
>  elf/get-dynamic-info.h | 41 ++++++++++++++++++-----------------------
>  1 file changed, 18 insertions(+), 23 deletions(-)
> 
> diff --git a/elf/get-dynamic-info.h b/elf/get-dynamic-info.h
> index 4f9aac63de..d8b071dff5 100644
> --- a/elf/get-dynamic-info.h
> +++ b/elf/get-dynamic-info.h
> @@ -28,52 +28,47 @@ static
>  auto
>  #endif
>  inline void __attribute__ ((unused, always_inline))
>  elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp)
>  {
> -  ElfW(Dyn) *dyn = l->l_ld;
> -  ElfW(Dyn) **info;
>  #if __ELF_NATIVE_CLASS == 32
>    typedef Elf32_Word d_tag_utype;
>  #elif __ELF_NATIVE_CLASS == 64
>    typedef Elf64_Xword d_tag_utype;
>  #endif
>  
>  #if !defined RTLD_BOOTSTRAP && !defined STATIC_PIE_BOOTSTRAP
> -  if (dyn == NULL)
> +  if (l->l_ld == NULL)
>      return;
>  #endif
>  
> -  info = l->l_info;
> +  ElfW(Dyn) **info = l->l_info;
>  

Ok.

> -  while (dyn->d_tag != DT_NULL)
> +  for (ElfW(Dyn) *dyn = l->l_ld; dyn->d_tag != DT_NULL; dyn++)
>      {
> +      d_tag_utype i;
> +
>        if ((d_tag_utype) dyn->d_tag < DT_NUM)
> -	info[dyn->d_tag] = dyn;
> +	i = dyn->d_tag;
>        else if (dyn->d_tag >= DT_LOPROC
>  	       && dyn->d_tag < DT_LOPROC + DT_THISPROCNUM)
> -	{
> -	  /* This does not violate the array bounds of l->l_info, but
> -	     gcc 4.6 on sparc somehow does not see this.  */
> -	  DIAG_PUSH_NEEDS_COMMENT;
> -	  DIAG_IGNORE_NEEDS_COMMENT (4.6,
> -				     "-Warray-bounds");
> -	  info[dyn->d_tag - DT_LOPROC + DT_NUM] = dyn;
> -	  DIAG_POP_NEEDS_COMMENT;
> -	}
> +	i = dyn->d_tag - DT_LOPROC + DT_NUM;

Ok.
>        else if ((d_tag_utype) DT_VERSIONTAGIDX (dyn->d_tag) < DT_VERSIONTAGNUM)
> -	info[VERSYMIDX (dyn->d_tag)] = dyn;
> +	i = DT_VERSIONTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM;

Why expand VERSYMIDX macro here?

>        else if ((d_tag_utype) DT_EXTRATAGIDX (dyn->d_tag) < DT_EXTRANUM)
> -	info[DT_EXTRATAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
> -	     + DT_VERSIONTAGNUM] = dyn;
> +	i = DT_EXTRATAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
> +	    + DT_VERSIONTAGNUM;

Ok.

>        else if ((d_tag_utype) DT_VALTAGIDX (dyn->d_tag) < DT_VALNUM)
> -	info[DT_VALTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
> -	     + DT_VERSIONTAGNUM + DT_EXTRANUM] = dyn;
> +	i = DT_VALTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
> +	    + DT_VERSIONTAGNUM + DT_EXTRANUM;

Ok.

>        else if ((d_tag_utype) DT_ADDRTAGIDX (dyn->d_tag) < DT_ADDRNUM)
> -	info[DT_ADDRTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
> -	     + DT_VERSIONTAGNUM + DT_EXTRANUM + DT_VALNUM] = dyn;
> -      ++dyn;
> +	i = DT_ADDRTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
> +	    + DT_VERSIONTAGNUM + DT_EXTRANUM + DT_VALNUM;
> +      else
> +	continue;
> +
> +      info[i] = dyn;
>      }
>  
>  #define DL_RO_DYN_TEMP_CNT	8
>  
>  #ifndef DL_RO_DYN_SECTION
> 

Ok.
Szabolcs Nagy Jan. 13, 2021, 9:10 a.m. UTC | #2 
The 01/12/2021 14:52, Adhemerval Zanella wrote:
> On 11/01/2021 12:11, Szabolcs Nagy via Libc-alpha wrote:
> > Since commit 2f056e8a5dd4dc0f075413f931e82cede37d1057
> > "aarch64: define PI_STATIC_AND_HIDDEN",
> > building glibc with gcc-8 on aarch64 fails with
> > 
> > /BLD/elf/librtld.os: in function `elf_get_dynamic_info':
> > /SRC/elf/get-dynamic-info.h:70:(.text+0xad8): relocation truncated to
> >  fit: R_AARCH64_ADR_PREL_PG_HI21 against symbol `_rtld_local' defined
> >  in .data section in /BLD/elf/librtld.os
> > 
> > This is a gcc bug:
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98618
> > 
> > Rewriting the affected code in elf_get_dynamic_info
> > seems to make this issue go away on old gcc.
> > 
> > The change makes the logic a bit clearer too (by
> > separating the index computation and array update)
> > and drops an older gcc workaround (since gcc 4.6
> > is no longer supported).
> 
> So if I understood correctly this bug affects only gcc-8? Or
> does it affect older version as well? From comment #3, Wilco stated

affects gcc-7 and older gcc too, but i see this
was not clear in my message.

> it was fixed by on gcc-10, so I take it was backported to gcc-9 
> already (since you didn't see it on gcc-9).

it seems to affect gcc-9 too, but a change in how
constants are constructed seems to make it less
likely to hit.

i'll update the description.

> 
> I did a sniff build tests with different gcc (8 and 9 mostly) on 
> different architectures and it seems ok.
> 
> LGTM with the just nit below about VERSYMIDX.
> 
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> 
...
> > -  while (dyn->d_tag != DT_NULL)
> > +  for (ElfW(Dyn) *dyn = l->l_ld; dyn->d_tag != DT_NULL; dyn++)
> >      {
> > +      d_tag_utype i;
> > +
> >        if ((d_tag_utype) dyn->d_tag < DT_NUM)
> > -	info[dyn->d_tag] = dyn;
> > +	i = dyn->d_tag;
> >        else if (dyn->d_tag >= DT_LOPROC
> >  	       && dyn->d_tag < DT_LOPROC + DT_THISPROCNUM)
> > -	{
> > -	  /* This does not violate the array bounds of l->l_info, but
> > -	     gcc 4.6 on sparc somehow does not see this.  */
> > -	  DIAG_PUSH_NEEDS_COMMENT;
> > -	  DIAG_IGNORE_NEEDS_COMMENT (4.6,
> > -				     "-Warray-bounds");
> > -	  info[dyn->d_tag - DT_LOPROC + DT_NUM] = dyn;
> > -	  DIAG_POP_NEEDS_COMMENT;
> > -	}
> > +	i = dyn->d_tag - DT_LOPROC + DT_NUM;
> 
> Ok.
> >        else if ((d_tag_utype) DT_VERSIONTAGIDX (dyn->d_tag) < DT_VERSIONTAGNUM)
> > -	info[VERSYMIDX (dyn->d_tag)] = dyn;
> > +	i = DT_VERSIONTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM;
> 
> Why expand VERSYMIDX macro here?
> 

for the same reason VALIDX and ADDRIDX are not used below:
i think this makes it clearer what's going on, you don't
have to look up these convenience macros in ldsodefs.h
to verify that the checks are right.

but i can undo this change.

> >        else if ((d_tag_utype) DT_EXTRATAGIDX (dyn->d_tag) < DT_EXTRANUM)
> > -	info[DT_EXTRATAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
> > -	     + DT_VERSIONTAGNUM] = dyn;
> > +	i = DT_EXTRATAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
> > +	    + DT_VERSIONTAGNUM;
> 
> Ok.
> 
> >        else if ((d_tag_utype) DT_VALTAGIDX (dyn->d_tag) < DT_VALNUM)
> > -	info[DT_VALTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
> > -	     + DT_VERSIONTAGNUM + DT_EXTRANUM] = dyn;
> > +	i = DT_VALTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
> > +	    + DT_VERSIONTAGNUM + DT_EXTRANUM;
> 
> Ok.
> 
> >        else if ((d_tag_utype) DT_ADDRTAGIDX (dyn->d_tag) < DT_ADDRNUM)
> > -	info[DT_ADDRTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
> > -	     + DT_VERSIONTAGNUM + DT_EXTRANUM + DT_VALNUM] = dyn;
> > -      ++dyn;
> > +	i = DT_ADDRTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
> > +	    + DT_VERSIONTAGNUM + DT_EXTRANUM + DT_VALNUM;
> > +      else
> > +	continue;
> > +
> > +      info[i] = dyn;
> >      }
> >  
> >  #define DL_RO_DYN_TEMP_CNT	8
> >  
> >  #ifndef DL_RO_DYN_SECTION
> > 
> 
> Ok.
Adhemerval Zanella Netto Jan. 13, 2021, 12:53 p.m. UTC | #3 
On 13/01/2021 06:10, Szabolcs Nagy wrote:
> The 01/12/2021 14:52, Adhemerval Zanella wrote:
>> On 11/01/2021 12:11, Szabolcs Nagy via Libc-alpha wrote:
>>> Since commit 2f056e8a5dd4dc0f075413f931e82cede37d1057
>>> "aarch64: define PI_STATIC_AND_HIDDEN",
>>> building glibc with gcc-8 on aarch64 fails with
>>>
>>> /BLD/elf/librtld.os: in function `elf_get_dynamic_info':
>>> /SRC/elf/get-dynamic-info.h:70:(.text+0xad8): relocation truncated to
>>>  fit: R_AARCH64_ADR_PREL_PG_HI21 against symbol `_rtld_local' defined
>>>  in .data section in /BLD/elf/librtld.os
>>>
>>> This is a gcc bug:
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98618
>>>
>>> Rewriting the affected code in elf_get_dynamic_info
>>> seems to make this issue go away on old gcc.
>>>
>>> The change makes the logic a bit clearer too (by
>>> separating the index computation and array update)
>>> and drops an older gcc workaround (since gcc 4.6
>>> is no longer supported).
>>
>> So if I understood correctly this bug affects only gcc-8? Or
>> does it affect older version as well? From comment #3, Wilco stated
> 
> affects gcc-7 and older gcc too, but i see this
> was not clear in my message.
> 
>> it was fixed by on gcc-10, so I take it was backported to gcc-9 
>> already (since you didn't see it on gcc-9).
> 
> it seems to affect gcc-9 too, but a change in how
> constants are constructed seems to make it less
> likely to hit.
> 
> i'll update the description.

Ok, thanks.

> 
>>
>> I did a sniff build tests with different gcc (8 and 9 mostly) on 
>> different architectures and it seems ok.
>>
>> LGTM with the just nit below about VERSYMIDX.
>>
>> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>>
> ...
>>> -  while (dyn->d_tag != DT_NULL)
>>> +  for (ElfW(Dyn) *dyn = l->l_ld; dyn->d_tag != DT_NULL; dyn++)
>>>      {
>>> +      d_tag_utype i;
>>> +
>>>        if ((d_tag_utype) dyn->d_tag < DT_NUM)
>>> -	info[dyn->d_tag] = dyn;
>>> +	i = dyn->d_tag;
>>>        else if (dyn->d_tag >= DT_LOPROC
>>>  	       && dyn->d_tag < DT_LOPROC + DT_THISPROCNUM)
>>> -	{
>>> -	  /* This does not violate the array bounds of l->l_info, but
>>> -	     gcc 4.6 on sparc somehow does not see this.  */
>>> -	  DIAG_PUSH_NEEDS_COMMENT;
>>> -	  DIAG_IGNORE_NEEDS_COMMENT (4.6,
>>> -				     "-Warray-bounds");
>>> -	  info[dyn->d_tag - DT_LOPROC + DT_NUM] = dyn;
>>> -	  DIAG_POP_NEEDS_COMMENT;
>>> -	}
>>> +	i = dyn->d_tag - DT_LOPROC + DT_NUM;
>>
>> Ok.
>>>        else if ((d_tag_utype) DT_VERSIONTAGIDX (dyn->d_tag) < DT_VERSIONTAGNUM)
>>> -	info[VERSYMIDX (dyn->d_tag)] = dyn;
>>> +	i = DT_VERSIONTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM;
>>
>> Why expand VERSYMIDX macro here?
>>
> 
> for the same reason VALIDX and ADDRIDX are not used below:
> i think this makes it clearer what's going on, you don't
> have to look up these convenience macros in ldsodefs.h
> to verify that the checks are right.
> 
> but i can undo this change.

I don't have a strong preference, but VERSYMIDX seems to be used in a
lot of place so it makes sense to be used where applicable.

> 
>>>        else if ((d_tag_utype) DT_EXTRATAGIDX (dyn->d_tag) < DT_EXTRANUM)
>>> -	info[DT_EXTRATAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
>>> -	     + DT_VERSIONTAGNUM] = dyn;
>>> +	i = DT_EXTRATAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
>>> +	    + DT_VERSIONTAGNUM;
>>
>> Ok.
>>
>>>        else if ((d_tag_utype) DT_VALTAGIDX (dyn->d_tag) < DT_VALNUM)
>>> -	info[DT_VALTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
>>> -	     + DT_VERSIONTAGNUM + DT_EXTRANUM] = dyn;
>>> +	i = DT_VALTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
>>> +	    + DT_VERSIONTAGNUM + DT_EXTRANUM;
>>
>> Ok.
>>
>>>        else if ((d_tag_utype) DT_ADDRTAGIDX (dyn->d_tag) < DT_ADDRNUM)
>>> -	info[DT_ADDRTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
>>> -	     + DT_VERSIONTAGNUM + DT_EXTRANUM + DT_VALNUM] = dyn;
>>> -      ++dyn;
>>> +	i = DT_ADDRTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
>>> +	    + DT_VERSIONTAGNUM + DT_EXTRANUM + DT_VALNUM;
>>> +      else
>>> +	continue;
>>> +
>>> +      info[i] = dyn;
>>>      }
>>>  
>>>  #define DL_RO_DYN_TEMP_CNT	8
>>>  
>>>  #ifndef DL_RO_DYN_SECTION
>>>
>>
>> Ok.
diff mbox series

Patch

diff --git a/elf/get-dynamic-info.h b/elf/get-dynamic-info.h
index 4f9aac63de..d8b071dff5 100644
--- a/elf/get-dynamic-info.h
+++ b/elf/get-dynamic-info.h
@@ -28,52 +28,47 @@  static
 auto
 #endif
 inline void __attribute__ ((unused, always_inline))
 elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp)
 {
-  ElfW(Dyn) *dyn = l->l_ld;
-  ElfW(Dyn) **info;
 #if __ELF_NATIVE_CLASS == 32
   typedef Elf32_Word d_tag_utype;
 #elif __ELF_NATIVE_CLASS == 64
   typedef Elf64_Xword d_tag_utype;
 #endif
 
 #if !defined RTLD_BOOTSTRAP && !defined STATIC_PIE_BOOTSTRAP
-  if (dyn == NULL)
+  if (l->l_ld == NULL)
     return;
 #endif
 
-  info = l->l_info;
+  ElfW(Dyn) **info = l->l_info;
 
-  while (dyn->d_tag != DT_NULL)
+  for (ElfW(Dyn) *dyn = l->l_ld; dyn->d_tag != DT_NULL; dyn++)
     {
+      d_tag_utype i;
+
       if ((d_tag_utype) dyn->d_tag < DT_NUM)
-	info[dyn->d_tag] = dyn;
+	i = dyn->d_tag;
       else if (dyn->d_tag >= DT_LOPROC
 	       && dyn->d_tag < DT_LOPROC + DT_THISPROCNUM)
-	{
-	  /* This does not violate the array bounds of l->l_info, but
-	     gcc 4.6 on sparc somehow does not see this.  */
-	  DIAG_PUSH_NEEDS_COMMENT;
-	  DIAG_IGNORE_NEEDS_COMMENT (4.6,
-				     "-Warray-bounds");
-	  info[dyn->d_tag - DT_LOPROC + DT_NUM] = dyn;
-	  DIAG_POP_NEEDS_COMMENT;
-	}
+	i = dyn->d_tag - DT_LOPROC + DT_NUM;
       else if ((d_tag_utype) DT_VERSIONTAGIDX (dyn->d_tag) < DT_VERSIONTAGNUM)
-	info[VERSYMIDX (dyn->d_tag)] = dyn;
+	i = DT_VERSIONTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM;
       else if ((d_tag_utype) DT_EXTRATAGIDX (dyn->d_tag) < DT_EXTRANUM)
-	info[DT_EXTRATAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
-	     + DT_VERSIONTAGNUM] = dyn;
+	i = DT_EXTRATAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
+	    + DT_VERSIONTAGNUM;
       else if ((d_tag_utype) DT_VALTAGIDX (dyn->d_tag) < DT_VALNUM)
-	info[DT_VALTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
-	     + DT_VERSIONTAGNUM + DT_EXTRANUM] = dyn;
+	i = DT_VALTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
+	    + DT_VERSIONTAGNUM + DT_EXTRANUM;
       else if ((d_tag_utype) DT_ADDRTAGIDX (dyn->d_tag) < DT_ADDRNUM)
-	info[DT_ADDRTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
-	     + DT_VERSIONTAGNUM + DT_EXTRANUM + DT_VALNUM] = dyn;
-      ++dyn;
+	i = DT_ADDRTAGIDX (dyn->d_tag) + DT_NUM + DT_THISPROCNUM
+	    + DT_VERSIONTAGNUM + DT_EXTRANUM + DT_VALNUM;
+      else
+	continue;
+
+      info[i] = dyn;
     }
 
 #define DL_RO_DYN_TEMP_CNT	8
 
 #ifndef DL_RO_DYN_SECTION