[PATCH] D46830: [llvm-objcopy] Add --keep-file-symbols option

[Paul Semel via Phabricator via llvm-commits]

paulsemel added inline comments.


================
Comment at: test/tools/llvm-objcopy/keep-file-symbols.test:37-48
+#STRIPALL: Symbols [
+#STRIPALL-NEXT:  Symbol {
+#STRIPALL-NEXT:    Name: foo
+#STRIPALL-NEXT:    Value: 0x0
+#STRIPALL-NEXT:    Size: 0
+#STRIPALL-NEXT:    Binding: Local
+#STRIPALL-NEXT:    Type: File
----------------
jhenderson wrote:
> paulsemel wrote:
> > jhenderson wrote:
> > > Wait, shouldn't there be a null symbol here? Null symbols are required... That's probably a bug unrelated to your change, but should be fixed ASAP.
> > Well, yes, I added a TODO for this in my previous patch, about this dummy symbol. We shouldn't pass it to the removeSymbols function as @jakehehrlich stated :)
> > Do you want me to work on it ? :)
> If you don't mind. What we currently generate is actually illegal ELF. I was under the impression that the TODO was for coming up with a better way for handling the null symbol, and I assumed that it wasn't being stripped (unless the whole table was), hence why I didn't comment on it before.
Well, you're actually right. The TODO was for this reason, definitely. But if we handle it the way we planned to handle it, this will also fix this issue :)


Repository:
  rL LLVM

https://reviews.llvm.org/D46830