[PATCH] D69093: [llvm-objcopy] --add-symbol: fix crash if SHT_SYMTAB does not exist

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/add-symbol-no-symtab.test:36
+# SEC2:      Index: 1
+# SEC2-NEXT: Name: .shstrtab
+# SEC2-NEXT: Type: SHT_STRTAB
----------------
grimar wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > grimar wrote:
> > > > ".shstrtab" != ".strtab"
> > > Yes, we overload `.shstrtab` (section names only) for more purposes (section names+symbol names). I think this is fine, but it definitely deserves a comment.
> > > 
> > > (Similarly, clang reuse .strtab for both section names and symbol names.)
> > > 
> > > GNU objcopy does not allow such object files (I haven't looked into details what sections (.strtab or .symtab) it expects).
> > > 
> > > ```
> > > % objcopy --add-symbol=abs1=1 t t2
> > > objcopy: error: the input file 'a' has no sections
> > > ```
> > To expand a bit more on this topic.
> > 
> > For better test coverage, we should have a test that places .shstrtab before .strtab so that we can check the created .symtab picks .strtab as its sh_link field. But I don't know any way to reorder sections.
> > 
> > Since rL238073, clang no longer produces .shstrtab and uses a unified .strtab for both section names and symbol names. I cannot write something like
> > 
> >       if (Sec.Type == ELF::SHT_STRTAB && Obj.SectionNames != &Sec)
> > 
> > because such use cases will fail. Again, I cannot test it with existing yaml2obj functionality (I am not sure it is useful enough to make it possible to test such scenarios... probably not worth it)
> I see, thanks for the explanation.
> For better test coverage, we should have a test that places .shstrtab before .strtab so that we can check the created .symtab picks .strtab as its sh_link field. But I don't know any way to reorder sections.

btw, I've realized that actually you can make a test for this with yaml2obj:

```
--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_REL
  Machine: EM_X86_64
Sections:
  - Name: .shstrtab
    Type: SHT_STRTAB
  - Name: .strtab
    Type: SHT_STRTAB
Symbols:
  - Name: a1
    Binding: STB_GLOBAL
```


```
Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
  [ 0]                   NULL             0000000000000000  00000000
       0000000000000000  0000000000000000           0     0     0
  [ 1] .shstrtab         STRTAB           0000000000000000  00000040
       000000000000001b  0000000000000000           0     0     0
  [ 2] .strtab           STRTAB           0000000000000000  0000005b
       0000000000000004  0000000000000000           0     0     0
  [ 3] .symtab           SYMTAB           0000000000000000  00000060
       0000000000000030  0000000000000018           2     1     8
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69093/new/

https://reviews.llvm.org/D69093