Page MenuHomePhabricator

[Clang][Coroutine][DebugInfo] Follow-up: reduce a tests ordering requirements
ClosedPublic

Authored by jmorse on Apr 12 2021, 4:11 AM.

Details

Summary

Hi,

The test added in D97533 (and modified by this patch) has some overly strict printed metadata ordering requirements, specifically the interleaving of DILocalVariable nodes and DILocation nodes. Unfortunately our downstream changes alter this order, which causes us trouble.

The patch below reduces the need for ordering: it picks out the DILocalVariable nodes being sought, in any order (CHECK-DAG), and doesn't examine any DILocations. The subsequent CHECK-NOTs are what's important: the test seeks to ensure a duplicate set of DILocalVariables aren't emitted in the same scope.

I've tested that this fails before 3a6a80b641bcf and passes afterwards.

Diff Detail

Event Timeline

jmorse created this revision.Apr 12 2021, 4:11 AM
jmorse requested review of this revision.Apr 12 2021, 4:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2021, 4:11 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I am not 100% convinced this has the desired effect. I believe what it does is:

  • Search for the first occurrence of each of "val" "moParam" and "mcParam"
  • After the last of the above occurrences, ensure there are no other occurrences of "val" "moParam" or "mcParam"

That is, it wouldn't fail on input of

"val"
"val"
"moParam"
"mcParam"

...because the second appearance of "val" is before the last text matched by one of the DAG directives.

I think really what you want is to specify the three variable names using {{--implicit-check-not}} on the FileCheck command, which (I am pretty sure) will apply to everything not explicitly matched by a CHECK directive.

@jmorse I am not familiar with CHECK-DAG. And @probinson 's suggestion might be useful, any ideas?

jmorse updated this revision to Diff 337793.Apr 15 2021, 9:33 AM

Right you are -- another thing I didn't clock was that this test was running all of the LLVM passes (generating many instances of the coroutine function). Latest revision disables those passes so that there are only DILocalVariables for the un-coro-split function. I've confirmed this fails before the fix landed and passes afterwards.

probinson accepted this revision.Apr 16 2021, 7:09 AM

One optional nit and LGTM, assuming the DILocalVariable ordering is consistent with what we see downstream.

clang/test/CodeGenCoroutines/coro-dwarf.cpp
1–4

I prefer the spelling -disable-llvm-passes

This revision is now accepted and ready to land.Apr 16 2021, 7:09 AM
This revision was landed with ongoing or failed builds.Apr 26 2021, 2:07 AM
This revision was automatically updated to reflect the committed changes.