[PATCH] D82935: [llvm-objcopy] Fix crash when removing symbol table at same time as adding a symbol

Fri Jul 3 05:53:59 PDT 2020

jubnzv marked an inline comment as done.
jubnzv added inline comments.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/add-symbol-new-symtab.test:44
+# CHECK-NEXT: [ 3] .shstrtab
+# CHECK-NEXT: [ 4] .symtab   {{.+}} {{.+}} {{.+}} {{.+}} {{.+}}      3  {{.+}}
+
----------------
jhenderson wrote:
> jubnzv wrote:
> > jhenderson wrote:
> > > Since this is a new section, I'd check the values are all correct, rather than skipping them. Don't forget the alignment column!
> > I agree. I think it will be more readable if I add values for the other columns as well.
> > The other tests for llvm-objcopy are written in the same way.
> It's not just about readability, it's about correctness. You could have added a new section called .symtab, with invalid properties easily (e.g. incorrect type, or a non-zero info field, for example). Without the explicit value checks, this would fail.
> 
> One exception is that you don't need to check the Offset field. The reason for that is because if we changed something internal in llvm-objcopy that changed the position the symbol table is put in the file, it would cause this test to break, but the Offset isn't actually something you control by simply adding the section, plus the ability to dump the symbol table validates that as a side-effect.
Understood, thanks for the explanation.
Interestingly, that the offset value generated by objcopy --add-section from GNU binutils and llvm-objcopy differs due to different internal implementation.

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

https://reviews.llvm.org/D82935