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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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  {{.+}}
+
----------------
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.


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

https://reviews.llvm.org/D82935