[PATCH] D141848: [Test] Fix YAML mapping keys duplication. NFC.

Wed Feb 1 01:53:08 PST 2023

jhenderson added a comment.

> @jhenderson , thanks for your suggestion. However, if I remove the second Type key, llvm-objcopy reports an error:
>
>   llvm-objcopy: error: 'build-release/test/tools/llvm-objcopy/ELF/Output/strip-all-gnu.test.tmp': string table '.strtab' cannot be removed because it is referenced by the symbol table '.symtab.dyn'
>
> Do you have any ideas what might be wrong?

Ah, yes, that would make (unfortunate) sense. So it looks like there are multiple issues:

1. There's a bug in the test whereby the .symtab.dyn section is being given the wrong section type. I tracked this down eventually to a change in the original patch introducing --strip-all (https://reviews.llvm.org/D39769?vs=122160&id=122524#toc), and it looks like a simple copy-paste or similar error. --strip-all was later changed to be --stip-all-gnu (with a new --strip-all taking its place), and the test (complete with bug) got migrated over. Looking at the original patch, and the relevant llvm-objcopy code, I'm 100% confident that .symtab.dyn is supposed to be of type SHT_SYMTAB.
2. When --strip-all-gnu is specified, llvm-objcopy attempts to remove any section of type SHT_STRTAB when that section is neither marked with SHF_ALLOC nor is the section header string table. By default, yaml2obj creates an implicit section called .strtab that any section of type SHT_SYMTAB references. It also emits an error when attempting to remove a section that is referenced by another section that is not being removed. Since, with my suggested change, .symtab.dyn is now of type SHT_SYMTAB, it will reference the implicit .strtab, but llvm-objcopy will try to remove it, and therefore produce the error. This wasn't picked up before because of the first issue.

For context, this test is largely testing the if block starting at this line: https://github.com/llvm/llvm-project/blob/9e845fe44fa125c482178c2b626693e5d80851e0/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp#L389. .symtab.dyn is supposed to show that a SHF_ALLOC section that would be removed due to its type is not removed (because it is marked as SHF_ALLOC and therefore triggers the early `return false` in the if block). A SHT_NOBITS section is not removed due to type, regardless of the SHF_ALLOC flag, so the section doesn't add anything to the test case in the current form of SHT_NOBITS + SHF_ALLOC. To maintain (or rather introduce) this test coverage, we should make .symtab.dyn SHT_SYMTAB, like I've suggested, but also make a preserved string section that it references. There are a few ways of doing this, but I suggest the following snippet to replace the existing .symtab.dyn section:

  - Name:            .symtab.dyn
    Type:            SHT_SYMTAB
    Link:            .strab.dyn
    Flags:           [ SHF_ALLOC ]
  - Name:            .strtab.dyn
    Type:            SHT_STRTAB
    Flags:           [ SHF_ALLOC ]

If you make that change to the YAML, you'll also need to add .strtab.dyn to the CHECK: list of sections that are preserved below the YAML, and increase the SectionHeaderCount check from 8 to 9.

Hopefully that all makes sense. You're welcome to spin this into a separate patch, if you want, to avoid confusing the other test changes, since this is a non-trivial change in the test.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/strip-all-gnu.test:28-29
   - Name:            .symtab.dyn
-    Type:            SHT_SYMTAB
-    Flags:           [ SHF_ALLOC ]
     Type:            SHT_NOBITS
+    Flags:           [ SHF_ALLOC ]
   - Name:            .text
----------------
asi-sc wrote:
> jhenderson wrote:
> > I don't think this is a correct change, based on my memory of this test and the --strip-all-gnu behaviour. I think it's likely the second Type key is actually spurious and should be deleted.
> @jhenderson , thanks for your suggestion. However, if I remove the second Type key, llvm-objcopy reports an error: 
> 
> ```
> llvm-objcopy: error: 'build-release/test/tools/llvm-objcopy/ELF/Output/strip-all-gnu.test.tmp': string table '.strtab' cannot be removed because it is referenced by the symbol table '.symtab.dyn'
> ```
> 
> Do you have any ideas what might be wrong?
Taking conversation out of line due to length.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/strip-all-gnu.test:28-29
   - Name:            .symtab.dyn
-    Type:            SHT_SYMTAB
-    Flags:           [ SHF_ALLOC ]
     Type:            SHT_NOBITS
+    Flags:           [ SHF_ALLOC ]
   - Name:            .text
----------------
jhenderson wrote:
> asi-sc wrote:
> > jhenderson wrote:
> > > I don't think this is a correct change, based on my memory of this test and the --strip-all-gnu behaviour. I think it's likely the second Type key is actually spurious and should be deleted.
> > @jhenderson , thanks for your suggestion. However, if I remove the second Type key, llvm-objcopy reports an error: 
> > 
> > ```
> > llvm-objcopy: error: 'build-release/test/tools/llvm-objcopy/ELF/Output/strip-all-gnu.test.tmp': string table '.strtab' cannot be removed because it is referenced by the symbol table '.symtab.dyn'
> > ```
> > 
> > Do you have any ideas what might be wrong?
> Taking conversation out of line due to length.
> @jhenderson , thanks for your suggestion. However, if I remove the second Type key, llvm-objcopy reports an error: 
> 
> ```
> llvm-objcopy: error: 'build-release/test/tools/llvm-objcopy/ELF/Output/strip-all-gnu.test.tmp': string table '.strtab' cannot be removed because it is referenced by the symbol table '.symtab.dyn'
> ```
> 
> Do you have any ideas what might be wrong?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141848