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