[MC] Improve st_size propagation rule

[MaskRay]

Summary

.symver foo, foo@ver creates the MCSymbolELF foo@ver whose almost all
attributes (including st_size) should inherit from foo (GNU as behavior).

a041ef1bd8905f0d58e301c6830b183002ff1847 added st_size propagation which works
for many cases but fails for the following one:

.set __GLIBC_2_12_sys_errlist, _sys_errlist_internal
.type   __GLIBC_2_12_sys_errlist,@object
.size   __GLIBC_2_12_sys_errlist, 1080
.symver __GLIBC_2_12_sys_errlist, sys_errlist@GLIBC_2.12
...
_sys_errlist_internal:
.size   _sys_errlist_internal, 1072

sys_errlist@GLIBC_2.12's st_size is 1072 (incorrect), which does not match
__GLIBC_2_12_sys_errlist's st_size: 1080.

The problem is that Base is (the final) _sys_errlist_internal while we want
to respect (the intermediate) __GLIBC_2_12_sys_errlist's st_size.
Fix this by following the MCSymbolRefExpr assignment chain and finding
the closest non-null getSize(), which covers most needs. Notably MCBinaryExpr
is not handled, but it is rare enough to matter.

[MaskRay]

drop an unneeded test change

[MaskRay]

llvm/test/MC/ELF/offset.s

50

[peter.smith]

I think this is an improvement. No objections from me. I had a quick thought about how we might be able to support the chained assignment case although it is only based on looking at this review so it could be flawed. Ideally we'd not wait to propagate the size in writeSymbol. I'm guessing that it is possible to write something like

.size x, 2
y = x
z = y
.size y, 1 // should also set size of z to 1.

So it is not as simple as propagating the size on assignment. Perhaps it would be some kind of size propagation step for all symbols prior to writing them.

llvm/lib/MC/ELFObjectWriter.cpp

556

Nit: which covers most needs or which can cover most needs

558

Would it be possible to follow the chain to get to the last non-zero size. This is assuming that in the case

.size x, 2
y = x
.size y, 1
z = y
z1 = z

*Sym = dyn_cast<MCSymbolRefExpr>(Symbol.getVariableValue(false) for z1 will give z
and Symbol.isVariable() is true for Sym, in that case can we use another if (auto *Sym = dyn_cast<MCSymbolRefExpr>(Symbol.getVariableValue(false)) to get to y, check that size is non zero and use that?

If this is possible I don't think it would be too much more complex.

[zatrazz]

I tested this patch on my glibc clang branch and while it has fixed the st_value aliases issues, it now triggers another regression:

$ cat /tmp/test.S 
.symver __GLIBC_2_1_sys_nerr, sys_nerr@GLIBC_2.2.5
.symver __GLIBC_2_1__sys_nerr, _sys_nerr@GLIBC_2.2.5

                                        # End of file scope inline assembly
        .type   __GLIBC_2_1_sys_nerr,@object    # @__GLIBC_2_1_sys_nerr
        .section        .rodata,"a",@progbits
        .globl  __GLIBC_2_1_sys_nerr
        .p2align        2
__GLIBC_2_1_sys_nerr:
        .long   125                             # 0x7d
        .size   __GLIBC_2_1_sys_nerr, 4

        .globl  __GLIBC_2_1__sys_nerr
.set __GLIBC_2_1__sys_nerr, __GLIBC_2_1_sys_nerr
$ clang /tmp/test.S -c -o /tmp/test-clang.os
$ $ readelf -s /tmp/test-clang.os  | grep _sys_nerr@
     4: 0000000000000000     0 OBJECT  GLOBAL DEFAULT    3 _sys_nerr@GLIBC_2.2.5

It should have the size '4' instead of '0'.

[MaskRay]

Implement the chained lookup. Fix zatrazz's case.

llvm/lib/MC/ELFObjectWriter.cpp

558

Thanks for the suggestion. Adopted.

[MaskRay]

So it is not as simple as propagating the size on assignment. Perhaps it would be some kind of size propagation step for all symbols prior to writing them.

Agree. The complexity is largely because the base symbol's size may be set after an assignment.

The size propagation basically needs to be done in a topological order (e.g. reverse post-order).
It can be implemented as a separate pass, or as the current approach does.
The current approach just has quadratic complexity in terms of the number of assignments. I think it is fine since MC already has such quadratic behaviors in several places (e.g. Asmparser "Recursive use of " error checking, Layout.getBaseSymbol(Symbol) just few lines above) and the complexity doesn't matter in practice (it's difficult to have a too-long assignment chain).
What's implemented here seems sufficient for a large number of use cases of using linker scripts symbol assignments and .symver.

[peter.smith]

Thanks for the update. Assuming zatrazz has no objections LGTM.

llvm/lib/MC/ELFObjectWriter.cpp

void ELFWriter::writeSymbol(SymbolTableWriter &Writer, uint32_t StringIndex,
   // 2 bits
   uint8_t Visibility = Symbol.getVisibility();
   uint8_t Other = Symbol.getOther() | Visibility;
 
   uint64_t Value = SymbolValue(*MSD.Symbol, Layout);
   uint64_t Size = 0;
 
   const MCExpr *ESize = MSD.Symbol->getSize();
-  if (!ESize && Base)
+  if (!ESize && Base) {
+    // For expressions like .set y, x+1, if y's size is unset, inherit from x.
     ESize = Base->getSize();
 
+    // For `.size x, 2; y = x; .size y, 1; z = y; z1 = z; .symver y, y@v1`, z,
+    // z1, and y@v1's st_size equals y's. However, `Base` is `x` which will give
+    // us 2. Follow the MCSymbolRefExpr assignment chain, which covers most
+    // needs. MCBinaryExpr is not handled.
+    const MCSymbolELF *Sym = &Symbol;
+    while (Sym->isVariable()) {
+      if (auto *Expr =
+              dyn_cast<MCSymbolRefExpr>(Sym->getVariableValue(false))) {
+        Sym = cast<MCSymbolELF>(&Expr->getSymbol());
+        if (!Sym->getSize())
+          continue;
+        ESize = Sym->getSize();
+      }
+      break;
+    }
+  }
+
   if (ESize) {
     int64_t Res;
     if (!ESize->evaluateKnownAbsolute(Res, Layout))
       report_fatal_error("Size expression must be absolute.");
     Size = Res;
   }
 
   // Write out the symbol table entry

llvm/test/MC/ELF/offset.s

 # RUN: llvm-mc -filetype=obj -triple x86_64 %s -o %t
 # RUN: llvm-readelf -s %t | FileCheck %s
 
 ## Test that a variable declared with "var = other_var + cst" is in the same
 ## section as other_var and its value is the value of other_var + cst.
 ## In addition, its st_size inherits from other_var.
 
 # CHECK:      0: {{.*}}
 # CHECK-NEXT:    0000000000000001    42 OBJECT  GLOBAL DEFAULT [[#A:]] a
 # CHECK-NEXT:    0000000000000005     0 NOTYPE  GLOBAL DEFAULT [[#A]]  b
 # CHECK-NEXT:    0000000000000001    42 OBJECT  GLOBAL DEFAULT [[#A]]  a1
 # CHECK-NEXT:    0000000000000002    42 OBJECT  GLOBAL DEFAULT [[#A]]  c
 # CHECK-NEXT:    000000000000000d    42 OBJECT  GLOBAL DEFAULT [[#A]]  d
 # CHECK-NEXT:    000000000000000d    42 OBJECT  GLOBAL DEFAULT [[#A]]  d1
 # CHECK-NEXT:    000000000000000d    42 OBJECT  GLOBAL DEFAULT [[#A]]  d2
 # CHECK-NEXT:    0000000000000001    41 OBJECT  GLOBAL DEFAULT [[#A]]  e
-# CHECK-NEXT:    0000000000000001    42 OBJECT  GLOBAL DEFAULT [[#A]]  e1
-# CHECK-NEXT:    0000000000000001    42 OBJECT  GLOBAL DEFAULT [[#A]]  e2
+# CHECK-NEXT:    0000000000000001    41 OBJECT  GLOBAL DEFAULT [[#A]]  e1
+# CHECK-NEXT:    0000000000000001    41 OBJECT  GLOBAL DEFAULT [[#A]]  e2
 # CHECK-NEXT:    0000000000000002    42 OBJECT  GLOBAL DEFAULT [[#A]]  e3
 # CHECK-NEXT:    0000000000000005     0 NOTYPE  GLOBAL DEFAULT [[#A]]  test2_a
 # CHECK-NEXT:    0000000000000005     0 NOTYPE  GLOBAL DEFAULT [[#A]]  test2_b
 # CHECK-NEXT:    0000000000000009     0 NOTYPE  GLOBAL DEFAULT [[#A]]  test2_c
 # CHECK-NEXT:    0000000000000009     0 NOTYPE  GLOBAL DEFAULT [[#A]]  test2_d
 # CHECK-NEXT:    0000000000000004     0 NOTYPE  GLOBAL DEFAULT  ABS    test2_e
-# CHECK-NEXT:    0000000000000001    42 OBJECT  GLOBAL DEFAULT [[#A]]  e@v1
+# CHECK-NEXT:    0000000000000001    41 OBJECT  GLOBAL DEFAULT [[#A]]  e@v1
 
 
         .data
         .globl  a
         .size a, 42
         .byte 42
         .type a, @object
 a:
 
         .long 42
         .globl b, a1, c, d, d1, d2, e, e1, e2, e3
 b:
 a1 = a
 c = a + 1
 
 ## These st_size fields inherit from a.
 d = a + (b - a) * 3
 .set d1, d
 d2 = d1
 
 e = a + (1 - 1)
 .size e, 41
-## FIXME These st_size fields inherit from e instead of a.
+## These st_size fields inherit from e instead of a.
+## TODO e3's st_size should inherit from e.
 .set e1, e
 .set e2, e1
 e3 = e1 + 1
 
         .globl test2_a
         .globl test2_b
         .globl test2_c
         .globl test2_d
         .globl test2_e