public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] gas: copy st_size only if unset
@ 2022-03-31  3:37 Fangrui Song
  2022-03-31  6:22 ` Jan Beulich
  0 siblings, 1 reply; 11+ messages in thread
From: Fangrui Song @ 2022-03-31  3:37 UTC (permalink / raw)
  To: binutils, Alan Modra, Nick Clifton

For
```
.size foo1, 1
foo1:

.set bar1, foo1
.size bar1, 2
.size bar2, 2
.set bar2, foo1

.set bar3, foo2
.size bar3, 2
.size bar4, 2
.set bar4, foo2

.size foo2, 1
foo2:
```

bar1's size is 2 while bar2, bar3, bar4's is 1. The behavior of bar1 makes sense
(generally directives on the new symbol should win) and is relied upon by glibc
stdio-common/errlist.c:

```
        .hidden _sys_errlist_internal
        .globl  _sys_errlist_internal
        .type   _sys_errlist_internal, @object
        .size   _sys_errlist_internal, 1072
_sys_errlist_internal:

        .globl __GLIBC_2_1_sys_errlist
        .set __GLIBC_2_1_sys_errlist, _sys_errlist_internal
        .type __GLIBC_2_1_sys_errlist, %object
        .size __GLIBC_2_1_sys_errlist, 125 * (64 / 8)

// glibc expects that .size __GLIBC_2_1_sys_errlist, 125 * (64 / 8) wins.
```

The behavior of bar2/bar3/bar4 seems brittle. To avoid the reordering of the two
code blocks which will result in the bar3 situation, glibc compiles errlist.c
with gcc -fno-toplevel-reorder (previously -fno-unit-at-a-time).

To fix the inconsistency and improve robustness, make bar2/bar3/bar4 match bar1,
removing the directive order sensitivity.

There is a pity that `.size dest, 0` is indistinguishable from the case where
dest is unset, but it should be fine.

    PR gas/29012
    * config/obj-elf.c (elf_copy_symbol_attributes): don't copy if src's size
      has been set.
    * testsuite/gas/elf/elf.exp: New test.
    * testsuite/gas/elf/size.d: New file.
    * testsuite/gas/elf/size.s: Likewise.
---
 gas/config/obj-elf.c          | 24 ++++++++++--------------
 gas/testsuite/gas/elf/elf.exp |  1 +
 gas/testsuite/gas/elf/size.d  | 13 +++++++++++++
 gas/testsuite/gas/elf/size.s  | 20 ++++++++++++++++++++
 4 files changed, 44 insertions(+), 14 deletions(-)
 create mode 100644 gas/testsuite/gas/elf/size.d
 create mode 100644 gas/testsuite/gas/elf/size.s

diff --git a/gas/config/obj-elf.c b/gas/config/obj-elf.c
index b56f8aa5220..b218ff6a574 100644
--- a/gas/config/obj-elf.c
+++ b/gas/config/obj-elf.c
@@ -2154,27 +2154,23 @@ elf_obj_symbol_clone_hook (symbolS *newsym, symbolS *orgsym ATTRIBUTE_UNUSED)
     }
 }
 
-/* When setting one symbol equal to another, by default we probably
-   want them to have the same "size", whatever it means in the current
-   context.  */
-
 void
 elf_copy_symbol_attributes (symbolS *dest, symbolS *src)
 {
   struct elf_obj_sy *srcelf = symbol_get_obj (src);
   struct elf_obj_sy *destelf = symbol_get_obj (dest);
-  if (srcelf->size)
+  /* If size is unset, copy size from src.  Because we don't track whether
+     .size has been used, we can't differentiate .size dest, 0 from the case
+     where dest's size is unset.  */
+  if (!destelf->size && S_GET_SIZE (dest) == 0)
     {
-      if (destelf->size == NULL)
-	destelf->size = XNEW (expressionS);
-      *destelf->size = *srcelf->size;
+      if (srcelf->size)
+	{
+	  destelf->size = XNEW (expressionS);
+	  *destelf->size = *srcelf->size;
+	}
+      S_SET_SIZE (dest, S_GET_SIZE (src));
     }
-  else
-    {
-      free (destelf->size);
-      destelf->size = NULL;
-    }
-  S_SET_SIZE (dest, S_GET_SIZE (src));
   /* Don't copy visibility.  */
   S_SET_OTHER (dest, (ELF_ST_VISIBILITY (S_GET_OTHER (dest))
 		      | (S_GET_OTHER (src) & ~ELF_ST_VISIBILITY (-1))));
diff --git a/gas/testsuite/gas/elf/elf.exp b/gas/testsuite/gas/elf/elf.exp
index 081ab4ec683..963730a87a5 100644
--- a/gas/testsuite/gas/elf/elf.exp
+++ b/gas/testsuite/gas/elf/elf.exp
@@ -277,6 +277,7 @@ if { [is_elf_format] } then {
     run_dump_test "section28"
     run_dump_test "section29"
     run_dump_test "sh-link-zero"
+    run_dump_test "size"
     run_dump_test "dwarf2-1" $dump_opts
     run_dump_test "dwarf2-2" $dump_opts
     run_dump_test "dwarf2-3" $dump_opts
diff --git a/gas/testsuite/gas/elf/size.d b/gas/testsuite/gas/elf/size.d
new file mode 100644
index 00000000000..12de8a70c39
--- /dev/null
+++ b/gas/testsuite/gas/elf/size.d
@@ -0,0 +1,13 @@
+#readelf: -sW
+#name: ELF symbol size
+
+#...
+ +[0-9]+: 0+ +1 +NOTYPE +LOCAL +DEFAULT +[0-9]+ +foo1
+ +[0-9]+: 0+ +2 +NOTYPE +LOCAL +DEFAULT +[0-9]+ +bar1
+ +[0-9]+: 0+ +2 +NOTYPE +LOCAL +DEFAULT +[0-9]+ +bar2
+ +[0-9]+: 0+ +2 +NOTYPE +LOCAL +DEFAULT +[0-9]+ +bar3
+ +[0-9]+: 0+ +1 +NOTYPE +LOCAL +DEFAULT +[0-9]+ +foo2
+ +[0-9]+: 0+ +2 +NOTYPE +LOCAL +DEFAULT +[0-9]+ +bar4
+ +[0-9]+: 0+ +1 +NOTYPE +LOCAL +DEFAULT +[0-9]+ +bar5
+ +[0-9]+: 0+ +1 +NOTYPE +LOCAL +DEFAULT +[0-9]+ +bar6
+#pass
diff --git a/gas/testsuite/gas/elf/size.s b/gas/testsuite/gas/elf/size.s
new file mode 100644
index 00000000000..b7729f9c3b3
--- /dev/null
+++ b/gas/testsuite/gas/elf/size.s
@@ -0,0 +1,20 @@
+.text
+
+.size foo1, 1
+foo1:
+
+.set bar1, foo1
+.size bar1, 2
+.size bar2, 2
+.set bar2, foo1
+
+.set bar3, foo2
+.size bar3, 2
+.size bar4, 2
+.set bar4, foo2
+
+.set bar5, foo1
+.set bar6, foo2
+
+.size foo2, 1
+foo2:
-- 
2.35.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gas: copy st_size only if unset
  2022-03-31  3:37 [PATCH] gas: copy st_size only if unset Fangrui Song
@ 2022-03-31  6:22 ` Jan Beulich
  2022-04-01  1:33   ` Fangrui Song
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2022-03-31  6:22 UTC (permalink / raw)
  To: Fangrui Song; +Cc: binutils, Alan Modra, Nick Clifton

On 31.03.2022 05:37, Fangrui Song wrote:
> For
> ```
> .size foo1, 1
> foo1:
> 
> .set bar1, foo1
> .size bar1, 2
> .size bar2, 2
> .set bar2, foo1
> 
> .set bar3, foo2
> .size bar3, 2
> .size bar4, 2
> .set bar4, foo2
> 
> .size foo2, 1
> foo2:
> ```
> 
> bar1's size is 2 while bar2, bar3, bar4's is 1. The behavior of bar1 makes sense
> (generally directives on the new symbol should win) and is relied upon by glibc
> stdio-common/errlist.c:

But what about

foo1:
	.size foo1, 1
foo2:
	.size foo2, 2
	.set bar, foo1
	.set bar, foo2

I would think bar's size should end up being 2 in this case, whereas I
think it would end up being 1 with your change.

Jan


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gas: copy st_size only if unset
  2022-03-31  6:22 ` Jan Beulich
@ 2022-04-01  1:33   ` Fangrui Song
  2022-04-04 11:27     ` Jan Beulich
  2022-04-08  7:42     ` Alan Modra
  0 siblings, 2 replies; 11+ messages in thread
From: Fangrui Song @ 2022-04-01  1:33 UTC (permalink / raw)
  To: Jan Beulich; +Cc: binutils, Alan Modra, Nick Clifton

On 2022-03-31, Jan Beulich wrote:
>On 31.03.2022 05:37, Fangrui Song wrote:
>> For
>> ```
>> .size foo1, 1
>> foo1:
>>
>> .set bar1, foo1
>> .size bar1, 2
>> .size bar2, 2
>> .set bar2, foo1
>>
>> .set bar3, foo2
>> .size bar3, 2
>> .size bar4, 2
>> .set bar4, foo2
>>
>> .size foo2, 1
>> foo2:
>> ```
>>
>> bar1's size is 2 while bar2, bar3, bar4's is 1. The behavior of bar1 makes sense
>> (generally directives on the new symbol should win) and is relied upon by glibc
>> stdio-common/errlist.c:
>
>But what about
>
>foo1:
>	.size foo1, 1
>foo2:
>	.size foo2, 2
>	.set bar, foo1
>	.set bar, foo2
>
>I would think bar's size should end up being 2 in this case, whereas I
>think it would end up being 1 with your change.
>
>Jan

bar's size is 2. The patch has no effect, because `dest` variables for the two
invocations are (surprisingly) different!

bar is a volatile symbol (created by .set instead of .equiv). The
second .set directive creates calls gas.c/read.c:3273 symbol_clone (symbolP, 1).

That said, I think it makes sense to add tests for this as well.
I'll use the following if this patch is accepted.

   .text
   
   .size foo1, 1
   foo1:
   
   .set bar1, foo1
   .size bar1, 2
   .size bar2, 2
   .set bar2, foo1
   
   .set bar3, foo2
   .size bar3, 2
   .size bar4, 2
   .set bar4, foo2
   
   .set bar5, foo1
   .set bar6, foo2
   
   .size foo2, 3
   foo2:
   
   .set bar7, foo1
   .set bar7, foo2

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gas: copy st_size only if unset
  2022-04-01  1:33   ` Fangrui Song
@ 2022-04-04 11:27     ` Jan Beulich
  2022-04-08  7:42     ` Alan Modra
  1 sibling, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2022-04-04 11:27 UTC (permalink / raw)
  To: Fangrui Song; +Cc: binutils, Alan Modra, Nick Clifton

On 01.04.2022 03:33, Fangrui Song wrote:
> On 2022-03-31, Jan Beulich wrote:
>> On 31.03.2022 05:37, Fangrui Song wrote:
>>> For
>>> ```
>>> .size foo1, 1
>>> foo1:
>>>
>>> .set bar1, foo1
>>> .size bar1, 2
>>> .size bar2, 2
>>> .set bar2, foo1
>>>
>>> .set bar3, foo2
>>> .size bar3, 2
>>> .size bar4, 2
>>> .set bar4, foo2
>>>
>>> .size foo2, 1
>>> foo2:
>>> ```
>>>
>>> bar1's size is 2 while bar2, bar3, bar4's is 1. The behavior of bar1 makes sense
>>> (generally directives on the new symbol should win) and is relied upon by glibc
>>> stdio-common/errlist.c:
>>
>> But what about
>>
>> foo1:
>> 	.size foo1, 1
>> foo2:
>> 	.size foo2, 2
>> 	.set bar, foo1
>> 	.set bar, foo2
>>
>> I would think bar's size should end up being 2 in this case, whereas I
>> think it would end up being 1 with your change.
>>
>> Jan
> 
> bar's size is 2. The patch has no effect, because `dest` variables for the two
> invocations are (surprisingly) different!
> 
> bar is a volatile symbol (created by .set instead of .equiv). The
> second .set directive creates calls gas.c/read.c:3273 symbol_clone (symbolP, 1).

Ah, yes.

> That said, I think it makes sense to add tests for this as well.
> I'll use the following if this patch is accepted.
> 
>    .text
>    
>    .size foo1, 1
>    foo1:
>    
>    .set bar1, foo1
>    .size bar1, 2
>    .size bar2, 2
>    .set bar2, foo1
>    
>    .set bar3, foo2
>    .size bar3, 2
>    .size bar4, 2
>    .set bar4, foo2
>    
>    .set bar5, foo1
>    .set bar6, foo2
>    
>    .size foo2, 3
>    foo2:
>    
>    .set bar7, foo1
>    .set bar7, foo2

Patch is okay with the extended testcase.

Jan


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gas: copy st_size only if unset
  2022-04-01  1:33   ` Fangrui Song
  2022-04-04 11:27     ` Jan Beulich
@ 2022-04-08  7:42     ` Alan Modra
  2022-04-08 21:26       ` Fangrui Song
  1 sibling, 1 reply; 11+ messages in thread
From: Alan Modra @ 2022-04-08  7:42 UTC (permalink / raw)
  To: Fangrui Song; +Cc: binutils

Your patch regressed these tests.  Please investigate.

aarch64_be-linux-gnu_ilp32  +FAIL: ELF symbol size
aarch64-elf  +FAIL: ELF symbol size
aarch64-linux  +FAIL: ELF symbol size
alpha-linux  +FAIL: ELF symbol size
alpha-netbsd  +FAIL: ELF symbol size
alpha-unknown-freebsd4.7  +FAIL: ELF symbol size
hppa64-hp-hpux11.23  +FAIL: ELF symbol size
hppa64-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
hppa-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
nds32be-elf  +FAIL: readelf -wiaoRlL dw5
nds32le-linux  +FAIL: readelf -wiaoRlL dw5
riscv32-elf  +FAIL: ELF symbol size
riscv64-linux  +FAIL: ELF symbol size

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gas: copy st_size only if unset
  2022-04-08  7:42     ` Alan Modra
@ 2022-04-08 21:26       ` Fangrui Song
  2022-04-08 23:01         ` Fangrui Song
                           ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Fangrui Song @ 2022-04-08 21:26 UTC (permalink / raw)
  To: Alan Modra; +Cc: binutils

On 2022-04-08, Alan Modra wrote:
>Your patch regressed these tests.  Please investigate.
>
>aarch64_be-linux-gnu_ilp32  +FAIL: ELF symbol size
>aarch64-elf  +FAIL: ELF symbol size
>aarch64-linux  +FAIL: ELF symbol size
>alpha-linux  +FAIL: ELF symbol size
>alpha-netbsd  +FAIL: ELF symbol size
>alpha-unknown-freebsd4.7  +FAIL: ELF symbol size
>hppa64-hp-hpux11.23  +FAIL: ELF symbol size
>hppa64-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
>hppa-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
>nds32be-elf  +FAIL: readelf -wiaoRlL dw5
>nds32le-linux  +FAIL: readelf -wiaoRlL dw5
>riscv32-elf  +FAIL: ELF symbol size
>riscv64-linux  +FAIL: ELF symbol size
>
>-- 
>Alan Modra
>Australia Development Lab, IBM

Sorry for the breakage. a3a7f5e1586467b137b8dcdcd2f74f5efa9f3919 should fix aarch64/alpha/hppa/riscv.
I am puzzled by 

> nds32be-elf  +FAIL: readelf -wiaoRlL dw5
> nds32le-linux  +FAIL: readelf -wiaoRlL dw5

../../configure CFLAGS='-O0 -g' CXXFLAGS='-O0 -g' --target=nds32be-elf
make -j 50 all-binutils && make -j 50 check-binutils
binutils/tmpdir/dw5.o does not change with "gas: copy st_size only if unset"

> hppa64-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
> hppa-linux  +XPASS: .reloc against undefined local symbol (PR 27228)

I am also puzzled by this. I confirm it passes on --target=hppa64-linux
I'll need to investigate it.


PS: is there a shortcut than building all-binutils/all-gas with various
different --target= (especially these exotic ones like
alpha/hppa/nds32)?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gas: copy st_size only if unset
  2022-04-08 21:26       ` Fangrui Song
@ 2022-04-08 23:01         ` Fangrui Song
  2022-04-08 23:06           ` Fangrui Song
  2022-04-09  1:12           ` Alan Modra
  2022-04-09  0:43         ` Alan Modra
  2022-04-09  4:29         ` Jeff Law
  2 siblings, 2 replies; 11+ messages in thread
From: Fangrui Song @ 2022-04-08 23:01 UTC (permalink / raw)
  To: Alan Modra; +Cc: binutils

On 2022-04-08, Fangrui Song wrote:
>On 2022-04-08, Alan Modra wrote:
>>Your patch regressed these tests.  Please investigate.
>>
>>aarch64_be-linux-gnu_ilp32  +FAIL: ELF symbol size
>>aarch64-elf  +FAIL: ELF symbol size
>>aarch64-linux  +FAIL: ELF symbol size
>>alpha-linux  +FAIL: ELF symbol size
>>alpha-netbsd  +FAIL: ELF symbol size
>>alpha-unknown-freebsd4.7  +FAIL: ELF symbol size
>>hppa64-hp-hpux11.23  +FAIL: ELF symbol size
>>hppa64-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
>>hppa-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
>>nds32be-elf  +FAIL: readelf -wiaoRlL dw5
>>nds32le-linux  +FAIL: readelf -wiaoRlL dw5
>>riscv32-elf  +FAIL: ELF symbol size
>>riscv64-linux  +FAIL: ELF symbol size
>>
>>-- 
>>Alan Modra
>>Australia Development Lab, IBM
>
>Sorry for the breakage. a3a7f5e1586467b137b8dcdcd2f74f5efa9f3919 should fix aarch64/alpha/hppa/riscv.
>I am puzzled by
>
>>nds32be-elf  +FAIL: readelf -wiaoRlL dw5
>>nds32le-linux  +FAIL: readelf -wiaoRlL dw5

OK, I think this is related to Nick's
19c26da69d68d5d863f37c06ad73ab6292d02ffa
("Add code to display the contents of .debug_loclists sections which contain offset entry tables.")

There is a diagnostic which has moved from somewhere in the middle to
the top:

   readelf: Warning: unable to apply unsupported type 208 to section .debug_loclists

Adding #... as the first line will fix the test for nds32*, but I am
unsure whether the test should be adjusted to unsupport nds32*.

>../../configure CFLAGS='-O0 -g' CXXFLAGS='-O0 -g' --target=nds32be-elf
>make -j 50 all-binutils && make -j 50 check-binutils
>binutils/tmpdir/dw5.o does not change with "gas: copy st_size only if unset"
>
>>hppa64-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
>>hppa-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
>
>I am also puzzled by this. I confirm it passes on --target=hppa64-linux
>I'll need to investigate it.
>
>
>PS: is there a shortcut than building all-binutils/all-gas with various
>different --target= (especially these exotic ones like
>alpha/hppa/nds32)?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gas: copy st_size only if unset
  2022-04-08 23:01         ` Fangrui Song
@ 2022-04-08 23:06           ` Fangrui Song
  2022-04-09  1:12           ` Alan Modra
  1 sibling, 0 replies; 11+ messages in thread
From: Fangrui Song @ 2022-04-08 23:06 UTC (permalink / raw)
  To: Alan Modra; +Cc: binutils

On 2022-04-08, Fangrui Song wrote:
>On 2022-04-08, Fangrui Song wrote:
>>On 2022-04-08, Alan Modra wrote:
>>>Your patch regressed these tests.  Please investigate.
>>>
>>>aarch64_be-linux-gnu_ilp32  +FAIL: ELF symbol size
>>>aarch64-elf  +FAIL: ELF symbol size
>>>aarch64-linux  +FAIL: ELF symbol size
>>>alpha-linux  +FAIL: ELF symbol size
>>>alpha-netbsd  +FAIL: ELF symbol size
>>>alpha-unknown-freebsd4.7  +FAIL: ELF symbol size
>>>hppa64-hp-hpux11.23  +FAIL: ELF symbol size
>>>hppa64-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
>>>hppa-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
>>>nds32be-elf  +FAIL: readelf -wiaoRlL dw5
>>>nds32le-linux  +FAIL: readelf -wiaoRlL dw5
>>>riscv32-elf  +FAIL: ELF symbol size
>>>riscv64-linux  +FAIL: ELF symbol size
>>>
>>>-- 
>>>Alan Modra
>>>Australia Development Lab, IBM
>>
>>Sorry for the breakage. a3a7f5e1586467b137b8dcdcd2f74f5efa9f3919 should fix aarch64/alpha/hppa/riscv.
>>I am puzzled by
>>
>>>nds32be-elf  +FAIL: readelf -wiaoRlL dw5
>>>nds32le-linux  +FAIL: readelf -wiaoRlL dw5
>
>OK, I think this is related to Nick's
>19c26da69d68d5d863f37c06ad73ab6292d02ffa
>("Add code to display the contents of .debug_loclists sections which contain offset entry tables.")
>
>There is a diagnostic which has moved from somewhere in the middle to
>the top:
>
>  readelf: Warning: unable to apply unsupported type 208 to section .debug_loclists
>
>Adding #... as the first line will fix the test for nds32*, but I am
>unsure whether the test should be adjusted to unsupport nds32*.
>
>>../../configure CFLAGS='-O0 -g' CXXFLAGS='-O0 -g' --target=nds32be-elf
>>make -j 50 all-binutils && make -j 50 check-binutils
>>binutils/tmpdir/dw5.o does not change with "gas: copy st_size only if unset"
>>
>>>hppa64-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
>>>hppa-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
>>
>>I am also puzzled by this. I confirm it passes on --target=hppa64-linux
>>I'll need to investigate it.

I have tested that the XPASS->PASS is unrelated to this particular gas patch.

>>
>>PS: is there a shortcut than building all-binutils/all-gas with various
>>different --target= (especially these exotic ones like
>>alpha/hppa/nds32)?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gas: copy st_size only if unset
  2022-04-08 21:26       ` Fangrui Song
  2022-04-08 23:01         ` Fangrui Song
@ 2022-04-09  0:43         ` Alan Modra
  2022-04-09  4:29         ` Jeff Law
  2 siblings, 0 replies; 11+ messages in thread
From: Alan Modra @ 2022-04-09  0:43 UTC (permalink / raw)
  To: Fangrui Song; +Cc: binutils

On Fri, Apr 08, 2022 at 02:26:13PM -0700, Fangrui Song wrote:
> On 2022-04-08, Alan Modra wrote:
> > Your patch regressed these tests.  Please investigate.
> > 
> > aarch64_be-linux-gnu_ilp32  +FAIL: ELF symbol size
> > aarch64-elf  +FAIL: ELF symbol size
> > aarch64-linux  +FAIL: ELF symbol size
> > alpha-linux  +FAIL: ELF symbol size
> > alpha-netbsd  +FAIL: ELF symbol size
> > alpha-unknown-freebsd4.7  +FAIL: ELF symbol size
> > hppa64-hp-hpux11.23  +FAIL: ELF symbol size
> > hppa64-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
> > hppa-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
> > nds32be-elf  +FAIL: readelf -wiaoRlL dw5
> > nds32le-linux  +FAIL: readelf -wiaoRlL dw5
> > riscv32-elf  +FAIL: ELF symbol size
> > riscv64-linux  +FAIL: ELF symbol size
> > 
> > -- 
> > Alan Modra
> > Australia Development Lab, IBM
> 
> Sorry for the breakage. a3a7f5e1586467b137b8dcdcd2f74f5efa9f3919 should fix aarch64/alpha/hppa/riscv.

Thanks for fixing these.

> I am puzzled by
> 
> > nds32be-elf  +FAIL: readelf -wiaoRlL dw5
> > nds32le-linux  +FAIL: readelf -wiaoRlL dw5
> 
> ../../configure CFLAGS='-O0 -g' CXXFLAGS='-O0 -g' --target=nds32be-elf
> make -j 50 all-binutils && make -j 50 check-binutils
> binutils/tmpdir/dw5.o does not change with "gas: copy st_size only if unset"

Hmm, I just did a couple of clean builds and confirmed the fail is
unrelated to your patch.  I'll look into why this test was failing for
me.

> > hppa64-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
> > hppa-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
> 
> I am also puzzled by this. I confirm it passes on --target=hppa64-linux
> I'll need to investigate it.
> 
> 
> PS: is there a shortcut than building all-binutils/all-gas with various
> different --target= (especially these exotic ones like
> alpha/hppa/nds32)?

No, because gas only supports one target, unlike many of the other
binutils that can be configured to support multiple targets.

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gas: copy st_size only if unset
  2022-04-08 23:01         ` Fangrui Song
  2022-04-08 23:06           ` Fangrui Song
@ 2022-04-09  1:12           ` Alan Modra
  1 sibling, 0 replies; 11+ messages in thread
From: Alan Modra @ 2022-04-09  1:12 UTC (permalink / raw)
  To: Fangrui Song; +Cc: binutils

On Fri, Apr 08, 2022 at 04:01:13PM -0700, Fangrui Song wrote:
> On 2022-04-08, Fangrui Song wrote:
> > On 2022-04-08, Alan Modra wrote:
> > > Your patch regressed these tests.  Please investigate.
> > > 
> > > aarch64_be-linux-gnu_ilp32  +FAIL: ELF symbol size
> > > aarch64-elf  +FAIL: ELF symbol size
> > > aarch64-linux  +FAIL: ELF symbol size
> > > alpha-linux  +FAIL: ELF symbol size
> > > alpha-netbsd  +FAIL: ELF symbol size
> > > alpha-unknown-freebsd4.7  +FAIL: ELF symbol size
> > > hppa64-hp-hpux11.23  +FAIL: ELF symbol size
> > > hppa64-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
> > > hppa-linux  +XPASS: .reloc against undefined local symbol (PR 27228)
> > > nds32be-elf  +FAIL: readelf -wiaoRlL dw5
> > > nds32le-linux  +FAIL: readelf -wiaoRlL dw5
> > > riscv32-elf  +FAIL: ELF symbol size
> > > riscv64-linux  +FAIL: ELF symbol size
> > > 
> > > -- 
> > > Alan Modra
> > > Australia Development Lab, IBM
> > 
> > Sorry for the breakage. a3a7f5e1586467b137b8dcdcd2f74f5efa9f3919 should fix aarch64/alpha/hppa/riscv.
> > I am puzzled by
> > 
> > > nds32be-elf  +FAIL: readelf -wiaoRlL dw5
> > > nds32le-linux  +FAIL: readelf -wiaoRlL dw5
> 
> OK, I think this is related to Nick's
> 19c26da69d68d5d863f37c06ad73ab6292d02ffa
> ("Add code to display the contents of .debug_loclists sections which contain offset entry tables.")
> 
> There is a diagnostic which has moved from somewhere in the middle to
> the top:
> 
>   readelf: Warning: unable to apply unsupported type 208 to section .debug_loclists
> 
> Adding #... as the first line will fix the test for nds32*, but I am
> unsure whether the test should be adjusted to unsupport nds32*.

You are exactly correct in your analysis.  Or we are both wrong in the
same way, because I independently came to the same conclusion.  :-)

The test shouldn't be adjusted.  It's now showing a defect in readelf
support for nds32 that was accidentally hidden before Nick's change.

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gas: copy st_size only if unset
  2022-04-08 21:26       ` Fangrui Song
  2022-04-08 23:01         ` Fangrui Song
  2022-04-09  0:43         ` Alan Modra
@ 2022-04-09  4:29         ` Jeff Law
  2 siblings, 0 replies; 11+ messages in thread
From: Jeff Law @ 2022-04-09  4:29 UTC (permalink / raw)
  To: binutils



On 4/8/2022 3:26 PM, Fangrui Song wrote:
>
>
>
> PS: is there a shortcut than building all-binutils/all-gas with various
> different --target= (especially these exotic ones like
> alpha/hppa/nds32)?
Not really.    But you can head to the upstream GCC tester and grab log 
files.  It happens to also test binutils on a variety of targets.

The tester recently moved out of AWS back to a machine I host.  So the 
redirect from the gcc web pages needs updating.   Anyway, point your 
browser here:

http://law-sandy.freeddns.org:8080/job/nds32le-elf/

 From that page we see that the build on April 5 was good and the build 
on April 6 was bad.  It flipped back to green today as I told the system 
generate new baselines for the nds targets as I'm primarily interested 
in catching GCC regressions.

You can dig into the log files to get the git hashes in use.  I use 
those to see my bisections when I need to chase something down.

Anyway, thought you might find it useful.

Jeff




^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-04-09  4:29 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-31  3:37 [PATCH] gas: copy st_size only if unset Fangrui Song
2022-03-31  6:22 ` Jan Beulich
2022-04-01  1:33   ` Fangrui Song
2022-04-04 11:27     ` Jan Beulich
2022-04-08  7:42     ` Alan Modra
2022-04-08 21:26       ` Fangrui Song
2022-04-08 23:01         ` Fangrui Song
2022-04-08 23:06           ` Fangrui Song
2022-04-09  1:12           ` Alan Modra
2022-04-09  0:43         ` Alan Modra
2022-04-09  4:29         ` Jeff Law

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).