[PATCH] D107324: [llvm-objcopy] [COFF] Do not patch debug entries with Repro type

[Pirama Arumuga Nainar via Phabricator via llvm-commits]

pirama marked 3 inline comments as done.
pirama added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/strip-brepro.test:2
+## Check that 'Repro' debug entry's non-existent debug directory payload is
+## handled correctly by `llvm-objcopy -S -x`.
+
----------------
jhenderson wrote:
> Why do you need both `-S` and `-x`? As I understand it, you only need one of them to strip the debug data.
> 
> Also, this comment needs enhancing to explain why this case is special.
> Why do you need both -S and -x? As I understand it, you only need one of them to strip the debug data.

This is a carry-over from our local build.  Neither -S nor -x is needed to reproduce the issue.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/strip-brepro.test:5
+# RUN: yaml2obj %s -o %t.exe
+# RUN: llvm-objcopy -S -x %t.exe
+
----------------
mstorsjo wrote:
> jhenderson wrote:
> > Shouldn't there be some check after this line to show that it's been stripped?
> I guess the intent of this test isn't to test stripping in itself, but that llvm-objcopy doesn't bail out while processing an executable with this content in it; the executable as such doesn't seem to contain anything that can be stripped out either. But making this clearer in the test comment would be good I guess.
Remove -S, -x.  The test is mainly for absence of error but I can add any relevant check on the output if needed.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107324