[PATCH] D85636: [llvm-dwarfdump] Fix misleading scope byte coverage statistics

[James Henderson via Phabricator via llvm-commits]

jhenderson added a subscriber: Higuoxing.
jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/stats-scope-bytes-covered.s:1-13
+# RUN: llvm-mc %s --filetype=obj -triple=x86_64-unknown-linux-gnu -o - \
+# RUN:   | llvm-dwarfdump --statistics - \
+# RUN:   | FileCheck %s
+#
+# Check that coverage for variable locations which do not cover the parent
+# scope is tracked separately in "sum_all_variables(#bytes in any scope)".
+#
----------------
Orlando wrote:
> dblaikie wrote:
> > Probably worth simplifying this test - it seems really long if it's only testing this property. (& if possible, including source code/steps to generate it - though probably better/easier to hand-craft DWARF for a case like this, since any case where LLVM produces locations wider than their scope is buggy & if that can't be tickled in a small example, maybe worth some hand-writing/tweaking of the DWARF or IR)
> I agree that the test is clunky, but I'm a bit stumped on what a good test would look like.
> 
> I would like to avoid testing from IR/MIR because, as you point out, the situation we are checking for is buggy output from clang. The output is prone to getting fixed and could cause a false failure in the future. That is why I chose to use assembly in the test, but it's not very human friendly, even with a smaller very contrived input MIR to generate the assembly.
> 
> yaml2obj seems like a promising solution but the yaml generated by obj2yaml is doesn't convey the purpose of the test either IMO.
> 
> So IMO a non-contrived (and more complex) yaml or .s test with the source in the comments is the way forward here, wdyt?
> 
I looked at this test, and I honestly can't see the wood from the trees. There's a lot of fluff in it that you can delete, as it has zero relevance to what you are testing. A YAML version might make it easier to trim it down further, although the DWARF support for ELF yaml2obj is still undergoing refinement - see the various patches by @Higuoxing in recent months. It might be worth switching to YAML, and then working with @Higuoxing to highlight where improvements could be made to yaml2obj to allow removing much of the useless bits - this is one of the aims of yaml2obj: to allow you to write concise tests that do what you need without things getting too verbose. The same can sometimes be achieved in assembly too, but raw assembly isn't that readable for DWARF, in my opinion.

Having the input code might help, but there's a risk it will go stale in the future.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/stats-scope-bytes-covered.s:5-6
+#
+# Check that coverage for variable locations which do not cover the parent
+# scope is tracked separately in "sum_all_variables(#bytes in any scope)".
+#
----------------
Tip: Something I'm trying to encourage is to use `##` for comments in lit tests (or equivalent for other comment markers), because it helps the comments stand out from the RUN and CHECK lines. Indeed, there's some suggestion that this should be adopted as "standard" practice to help FileCheck diagnostics (which for example here might complain about the use of "Check" in the first line not being a real CHECK directive).


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

https://reviews.llvm.org/D85636