[PATCH] D140084: [llvm][test] Split DW_AT_default_value check out of clang/test/

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D140084#3997851 <https://reviews.llvm.org/D140084#3997851>, @Michael137 wrote:

> In D140084#3997123 <https://reviews.llvm.org/D140084#3997123>, @dblaikie wrote:
>
>> Sorry, should've caught this in review. The clang change needs test coverage in clang, but should verify the emitted it, rather than going all the way down to object code.
>>
>> The llvm functionality is already tested (since it's just the flag on a template parameter - it's not interesting to test that for different kinds of templates if the flag handling is kind-agnostic anyway)
>
> I couldn't find any test inside `llvm/test` which verifies that `DW_AT_default_value` is being emitted correctly. Sure we don't need a test like that?

I tried disabling the emission of `DW_AT_default_value` and running `check-llvm` to see what fails:

  LLVM :: DebugInfo/X86/debug-info-default-template-parameter.ll
  LLVM :: DebugInfo/X86/debug-info-template-parameter.ll

So, probably is tested already? (well, I guess the former is the one you just added, but the latter was already present - the latter could be modified to check the strict-dwarf behavior, but it's not a problem that you've added another test for that instead of trying to multiplex the existing test - all good either way, really)

>> So instead of this could you adjust the existing clang test to verify IR instead of dwarfdump?
>
> @dblaikie Isn't the clang test already doing that? In this test I mainly wanted to check that the `gstrict-dwarf` attribute works as expected. The IR will always have the default attribute attached to it, which the `clang/test/` already tests.

So the `-gstrict-dwarf` clang-side is somewhat hard to test because this property doesn't get encoded in the IR, it's only passed through as an TargetOption (this is generally not "ideal", and we'd rather use at least a module metadata, or a flag on the DICompileUnit - but tradeoffs be tradeoffs). Yeah, we either don't test  it, or do make an exception and do an end-to-end test.

In this case, probably leave it untested at the Clang level.

At the LLVM level - it's testable, as you've found with the `-strict-dwarf=true` flag for llc.

(side note: when you send something for review through phab, please wait for approval before committing (otherwise it gets hard to tell what needed precommit review and may've been committed without it, versus things that weren't intended to be precommit reviewed - especially we don't want to give the impression that if you send something for review and don't receive a review quickly that it's OK to commit the patch anyway) - in this case, it might've been best to revert the original patch causing regressions if you weren't sure how to make a quick fix without precommit review - or to commit the fix directly & discuss it further post-commit if you wanted some more feedback to see if that was the ideal direction)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140084