[PATCH] D85024: [llvm-objdump] Implement --prefix option

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Please remember to update the documentation located at llvm/docs/CommandGuide for llvm-objdump, to include the new option.



================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-archive-with-modified-source-path.ll:1
+; This test checks if 'llvm-objdump' works as GNU's 'objdump' with '--prefix'
+; option.
----------------
tinti wrote:
> MaskRay wrote:
> > Please don't just copy `disassemble-archive-with-source.ll`
> > 
> > The test is named `disassemble-archive-with-modified-source-path.ll`
> > I wonder whether using an `archive` is significant to the test. If not, just use a plain `.o`
> > 
> > If an archive can trigger a slightly different code path you also want to test, you can add a second llvm-objdump RUN line for an archive.
> > 
> > Consider reusing an existing .ll file and not duplicating LLVM IR, e.g. source-interleave-x86_64.test
> > Please don't just copy `disassemble-archive-with-source.ll`
> > 
> > The test is named `disassemble-archive-with-modified-source-path.ll`
> > I wonder whether using an `archive` is significant to the test. If not, just use a plain `.o`
> 
> Understood. I believe the both `.o` and `.c` are needed.
>  
> > If an archive can trigger a slightly different code path you also want to test, you can add a second llvm-objdump RUN line for an archive.
> > 
> > Consider reusing an existing .ll file and not duplicating LLVM IR, e.g. source-interleave-x86_64.test
> 
> I will. The `source-interleave-x86_64.test` has `source-interleave-x86_64.c` too.
> I will. The `source-interleave-x86_64.test` has `source-interleave-x86_64.c` too.

That test to me sounds like the right one to be basing your test off-of. I agree the archive part doesn't sound important.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-archive-with-modified-source-path.ll:10
+; RUN: echo -e "int foo(int a)\n\n{ return a+1; }" > subdir/a.c
+; RUN: sed -e "s,DIRNAME,%/t/subdir," %s | llc --filetype=obj -mtriple=x86_64-pc-linux -o a.o
+; RUN: llvm-ar rc a.a a.o
----------------
tinti wrote:
> rengolin wrote:
> > These are Unix tools and not always available on Windows (PowerShell emulate some but not all behaviour). Do we need a tag to make sure it only runs on platforms we know will cope with the command lines above?
> > 
> > I'm not sure anyone cares about GNU compatibility on Windows anyway. :)
> Agreed. But I believe in this case I need to fix.
> 
> Current:
> - myprefixC:/ws/w16c2-1
> 
> Expected:
> - C:/myprefix/ws/w16c2-1
> 
>            8: c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\llvm-objdump.exe: warning: 'a.o': failed to find source myprefixC:/ws/w16c2-1/llvm-project/premerge-checks/build/test/tools/llvm-objdump/X86/Output/disassemble-archive-with-modified-source-path.ll.tmp/subdir\a.c
`sed` is one of the required tools to run the lit test suite on Windows. There are multiple other tests that have similar tools.

I care about GNU compatibility on Windows - we ship LLVM-based tools for Windows developers such as llvm-objdump, and the option seems useful to me!


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

https://reviews.llvm.org/D85024