[PATCH] D78888: [llvm-objcopy][MachO] Fix symbol table

[Alexander Shaposhnikov via Phabricator via llvm-commits]

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


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/stab-ext-symbol.test:4-10
+## static int x = 0;
+## // Defined external symbol
+## int main() {
+##   return x;
+## }
+## build command:
+##   clang -g -fno-exceptions -fno-unwind-tables a.c -o a.exe
----------------
jhenderson wrote:
> I wonder if you started from assembly whether you'd be able to reduce this test-case down further? It seems like many of the symbols are unnecessary, for example. I don't know enough about this to be able to assist unfortunately.
> 
> Also, is there any particular reason you can't just add this case to symbol-table.test?
@jhenderson - i understand you desire to reduce the test case further, there are a few things to keep in mind. 
1. Those symbols (SymDebugTable*) actually appear to be created by the linker, so assembly is not of much help (otherwise, in the test we would have to call the system linker, this is problematic in many ways even if one restricts this test to OSX). 
2.  I don't want to put this into symbol-table.test for multiple reasons. First, strictly speaking the value of n_type is invalid here and I don't like the idea of using invalid binaries implicitly, i.e. outside of the tests which expect them explicitly. Plus I'd like symbol-table.test to be as close to the real binary as possible.
 MachOReader could probably do some validation of these fields (MachOReader is essentially a wrapper around libObject so it's also sort of questionable) + new values of this field potentially can be added in the future.
This is actually another reason why I'm not sure this extra testing brings **a lot of** value in the first place (the rationale you mentioned in the comment makes some sense (so perhaps it brings **some** value), but this seems to be relatively minor, we've got many such places in LLVM). 
3. If you see a lot of value in this artificial test - since it's already hand-crafted we can probably try to reduce it manually a bit further.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78888