[PATCH] D151932: Add dsymutil dwarf5 tests for darwin

[Adrian Prantl via Phabricator via llvm-commits]

aprantl requested changes to this revision.
aprantl added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/test/tools/dsymutil/ARM/dwarf5-darwin.test:1
+RUN: rm -rf %t.dir && mkdir -p %t.dir
+RUN: yaml2obj %p/../Inputs/dwarf5-darwin.yaml -o %t.dir/dwarf5-darwin.o
----------------
avl wrote:
> bulbazord wrote:
> > avl wrote:
> > > probably, it should have
> > > ```
> > > # REQUIRES: system-darwin
> > > ```
> > > ?
> > My understanding is that shouldn't be needed. You can run most dsymutil tests on non-darwin systems.
> The test is named as if it has something Darwin specific. If it is not then, probably, it would be better to name it without mentioning Darwin. Also, it would be good if the test would describe itself in the very beginning. Something like:
> 
> 
> ```
> ## This test checks that simple MachO binary contains consistent DWARFv5 info.
> ```
> 
> or 
> 
> 
> ```
> ## This test checks that dsymutil generates the correct headers and sections for a MachO binary linked with one object file containing DWARFv4 and the other containing DWARFv5.
> 
> ```
+1 on commenting what the test does.

As for naming the test, we could call it `-macho` instead of `-darwin` since it's about the container format more than the platform?

As @bulbazord said, the REQUIRES line should not be needed.


================
Comment at: llvm/test/tools/dsymutil/ARM/dwarf5-darwin.test:3
+RUN: yaml2obj %p/../Inputs/dwarf5-darwin.yaml -o %t.dir/dwarf5-darwin.o
+RUN: dsymutil %t.dir/dwarf5-darwin.o -o %t.dir/dwarf5-darwin.dSYM
+RUN: llvm-dwarfdump %t.dir/dwarf5-darwin.dSYM -a --verbose | FileCheck %s
----------------
Either I'm confused by this test, or the test is doing something stange, but you can probably get rid of the .yaml file entirely.

dsymutil takes an executable mach-o binary (or a dylib) as input. So the input file's extension should probably not be `.o`.
But we also shouldn't need to create one, because dsymutil can take a debug map (~ what dsymutil -s would print) in yaml as input. This is why most tests just use `-y %p/dummy-debug-map.map`, which is a debug map that looks for a files named 1.o 2.o 3.o. So if you name your files accordingly (look at what the other tests are doing), you should be able to do the same.


================
Comment at: llvm/test/tools/dsymutil/ARM/dwarf5-dwarf4-comb.test:1
+RUN: rm -rf %t.dir && mkdir -p %t.dir
+RUN: yaml2obj %p/../Inputs/dwarf5-dwarf4-darwin.yaml -o %t.dir/dwarf5-dwarf4-darwin.o
----------------
I assume `-comb` stands for `combined`? Maybe just spell it out?


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

https://reviews.llvm.org/D151932