From 867b8c308a40e79f90db4051cafc273f00f881fb Mon Sep 17 00:00:00 2001 From: Fangrui Song Date: Mon, 4 Apr 2022 08:43:50 -0700 Subject: gas: copy st_size only if unset 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 the compromise seems 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 | 14 ++++++++++++++ gas/testsuite/gas/elf/size.s | 23 +++++++++++++++++++++++ 4 files changed, 48 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 b56f8aa..b218ff6 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 (destelf->size == NULL) - destelf->size = XNEW (expressionS); - *destelf->size = *srcelf->size; - } - else + /* 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) { - free (destelf->size); - destelf->size = NULL; + if (srcelf->size) + { + destelf->size = XNEW (expressionS); + *destelf->size = *srcelf->size; + } + S_SET_SIZE (dest, S_GET_SIZE (src)); } - 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 081ab4e..963730a 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 0000000..51bf4eb --- /dev/null +++ b/gas/testsuite/gas/elf/size.d @@ -0,0 +1,14 @@ +#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+ +3 +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+ +3 +NOTYPE +LOCAL +DEFAULT +[0-9]+ +bar6 + +[0-9]+: 0+ +3 +NOTYPE +LOCAL +DEFAULT +[0-9]+ +bar7 +#pass diff --git a/gas/testsuite/gas/elf/size.s b/gas/testsuite/gas/elf/size.s new file mode 100644 index 0000000..9286556 --- /dev/null +++ b/gas/testsuite/gas/elf/size.s @@ -0,0 +1,23 @@ +.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 -- cgit v1.1