[PATCH] D125783: [llvm-debuginfo-analyzer] 08 - ELF Reader

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/CMakeLists.txt:9
 
-add_llvm_unittest(DebugInfoLogicalViewTests
+add_llvm_unittest_with_input_files(DebugInfoLogicalViewTests
   CommandLineOptionsTest.cpp
----------------
thakis wrote:
> CarlosAlbertoEnciso wrote:
> > CarlosAlbertoEnciso wrote:
> > > thakis wrote:
> > > > See:
> > > > 
> > > > ```
> > > > # Use for test binaries that call llvm::getInputFileDirectory(). Use of this
> > > > # is discouraged.
> > > > function(add_unittest_with_input_files test_suite test_name)
> > > > ```
> > > > 
> > > > Any chance you could write this test without needing getInputFileDirectory()?
> > > There is only one remaining patch (09-codeview-reader) for the `llvm-debuginfo-analyzer` tool; it changes this file to include the `CodeViewReaderTest.cpp` which uses the `getInputFileDirectory` call.
> > > 
> > > I will explore the option to remove the call to `getInputFileDirectory` in that patch.
> > I was not able to rewrite the test without using `getInputFileDirectory`. Looked at other test cases that include a binary file and they use the same approach.
> Any chance you could use a lit test instead of a unit test for this then?
Yeah, seems like either a lit test (if this library is predominantly/sufficiently exposed via a command line utility - we generally test via lit tests on that corresponding command line utility (eg: other libraries in lib/DebugInfo are tested via llvm-dwarfdump and llvm-symbolizer, mostly))

Alternatively, if there's nuanced uses of the library not readily accessible via the command line (or easier to test once at a lower layer) - like some parts of libDebugInfoDWARF - are tested via unit tests that generate the DWARF they're testing within the unit test, rather than depending on a checked in binary. (see DWARFGenerator usage in the unit tests fo rexamples of this)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125783