[PATCH] D34673: [ELF] - Do not set st_size field of SHT_UNDEF symbols

[Rafael Avila de Espindola via llvm-commits]

LGTM

Thanks!

George Rimar via Phabricator <reviews at reviews.llvm.org> writes:

> grimar updated this revision to Diff 104110.
> grimar added a comment.
>
> - Added forgotten input file.
>
>
> https://reviews.llvm.org/D34673
>
> Files:
>   ELF/SyntheticSections.cpp
>   test/ELF/Inputs/dso-undef-size.s
>   test/ELF/dso-undef-size.s
>
>
> Index: test/ELF/dso-undef-size.s
> ===================================================================
> --- test/ELF/dso-undef-size.s
> +++ test/ELF/dso-undef-size.s
> @@ -0,0 +1,32 @@
> +# REQUIRES: x86
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %S/Inputs/dso-undef-size.s -o %t1.o
> +# RUN: ld.lld -shared %t1.o -o %t1.so
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t2.o
> +# RUN: ld.lld -shared %t2.o %t1.so -o %t2.so
> +# RUN: llvm-readobj -symbols -dyn-symbols %t2.so
> +
> +# CHECK:      Symbols [
> +# CHECK-NEXT:   Symbol {
> +# CHECK-NEXT:     Name: foo
> +# CHECK-NEXT:     Value:
> +# CHECK-NEXT:     Size: 0
> +# CHECK-NEXT:     Binding:
> +# CHECK-NEXT:     Type:
> +# CHECK-NEXT:     Other:
> +# CHECK-NEXT:     Section: Undefined
> +# CHECK-NEXT:   }
> +# CHECK-NEXT: ]
> +# CHECK:      DynamicSymbols [
> +# CHECK-NEXT:   Symbol {
> +# CHECK-NEXT:     Name: foo
> +# CHECK-NEXT:     Value:
> +# CHECK-NEXT:     Size: 0
> +# CHECK-NEXT:     Binding:
> +# CHECK-NEXT:     Type:
> +# CHECK-NEXT:     Other:
> +# CHECK-NEXT:     Section: Undefined
> +# CHECK-NEXT:   }
> +# CHECK-NEXT: ]
> +
> +.text
> +.global foo
> Index: test/ELF/Inputs/dso-undef-size.s
> ===================================================================
> --- test/ELF/Inputs/dso-undef-size.s
> +++ test/ELF/Inputs/dso-undef-size.s
> @@ -0,0 +1,4 @@
> +.text
> +.global foo
> +.size foo, 4
> +foo:
> Index: ELF/SyntheticSections.cpp
> ===================================================================
> --- ELF/SyntheticSections.cpp
> +++ ELF/SyntheticSections.cpp
> @@ -1376,7 +1376,6 @@
>      }
>  
>      ESym->st_name = Ent.StrTabOffset;
> -    ESym->st_size = Body->getSize<ELFT>();
>  
>      // Set a section index.
>      if (const OutputSection *OutSec = Body->getOutputSection())
> @@ -1386,6 +1385,13 @@
>      else if (isa<DefinedCommon>(Body))
>        ESym->st_shndx = SHN_COMMON;
>  
> +    // Do not set size for undefined symbols. This size is unreliable for case
> +    // when linking DSO which links to another DSO. In that case it is possible
> +    // that low level DSO may be changed in the way that alters symbol size. In
> +    // such case if high-level DSO is rebuilt, its CRC will change, what may be
> +    // undesirable. It is possible to avoid if we set st_size to zero.
> +    ESym->st_size = (ESym->st_shndx == SHN_UNDEF) ? 0 : Body->getSize<ELFT>();
> +
>      // st_value is usually an address of a symbol, but that has a
>      // special meaining for uninstantiated common symbols (this can
>      // occur if -r is given).
>
>
> Index: test/ELF/dso-undef-size.s
> ===================================================================
> --- test/ELF/dso-undef-size.s
> +++ test/ELF/dso-undef-size.s
> @@ -0,0 +1,32 @@
> +# REQUIRES: x86
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %S/Inputs/dso-undef-size.s -o %t1.o
> +# RUN: ld.lld -shared %t1.o -o %t1.so
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t2.o
> +# RUN: ld.lld -shared %t2.o %t1.so -o %t2.so
> +# RUN: llvm-readobj -symbols -dyn-symbols %t2.so
> +
> +# CHECK:      Symbols [
> +# CHECK-NEXT:   Symbol {
> +# CHECK-NEXT:     Name: foo
> +# CHECK-NEXT:     Value:
> +# CHECK-NEXT:     Size: 0
> +# CHECK-NEXT:     Binding:
> +# CHECK-NEXT:     Type:
> +# CHECK-NEXT:     Other:
> +# CHECK-NEXT:     Section: Undefined
> +# CHECK-NEXT:   }
> +# CHECK-NEXT: ]
> +# CHECK:      DynamicSymbols [
> +# CHECK-NEXT:   Symbol {
> +# CHECK-NEXT:     Name: foo
> +# CHECK-NEXT:     Value:
> +# CHECK-NEXT:     Size: 0
> +# CHECK-NEXT:     Binding:
> +# CHECK-NEXT:     Type:
> +# CHECK-NEXT:     Other:
> +# CHECK-NEXT:     Section: Undefined
> +# CHECK-NEXT:   }
> +# CHECK-NEXT: ]
> +
> +.text
> +.global foo
> Index: test/ELF/Inputs/dso-undef-size.s
> ===================================================================
> --- test/ELF/Inputs/dso-undef-size.s
> +++ test/ELF/Inputs/dso-undef-size.s
> @@ -0,0 +1,4 @@
> +.text
> +.global foo
> +.size foo, 4
> +foo:
> Index: ELF/SyntheticSections.cpp
> ===================================================================
> --- ELF/SyntheticSections.cpp
> +++ ELF/SyntheticSections.cpp
> @@ -1376,7 +1376,6 @@
>      }
>  
>      ESym->st_name = Ent.StrTabOffset;
> -    ESym->st_size = Body->getSize<ELFT>();
>  
>      // Set a section index.
>      if (const OutputSection *OutSec = Body->getOutputSection())
> @@ -1386,6 +1385,13 @@
>      else if (isa<DefinedCommon>(Body))
>        ESym->st_shndx = SHN_COMMON;
>  
> +    // Do not set size for undefined symbols. This size is unreliable for case
> +    // when linking DSO which links to another DSO. In that case it is possible
> +    // that low level DSO may be changed in the way that alters symbol size. In
> +    // such case if high-level DSO is rebuilt, its CRC will change, what may be
> +    // undesirable. It is possible to avoid if we set st_size to zero.
> +    ESym->st_size = (ESym->st_shndx == SHN_UNDEF) ? 0 : Body->getSize<ELFT>();
> +
>      // st_value is usually an address of a symbol, but that has a
>      // special meaining for uninstantiated common symbols (this can
>      // occur if -r is given).