[PATCH] D90772: [Coroutines] Add missing llvm.dbg.declare's to cover more allocas

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added subscribers: Orlando, chrisjackson.
jmorse added a comment.

bruno wrote:

> That's a good question, I initially thought a dbg.declare dominating the BB in question would be enough, perhaps the debugging experts could help clarify what's going on.

Hrrrmmmm. Just to confirm my understanding, in tests like the one modified in this patch, all those allocas (%i, %j, %x) are transformed into GEPs from FramePtr, which is malloc'd / new'd or otherwise not on the stack?

Normally dbg.declare is supposed to refer to a static alloca -- that way we can easily identify stack variables and not bother tracking them through optimisations. Sometimes this isn't true though (certain ABIs pass in memory for return values apparently), in which case the dbg.declare gets silently transformed into something like `dbg.value(%0, [...], DW_OP_deref)`, which is what seems to happen here.

That should be fine: however as you've already spotted, the debug intrinsic needs to dominate the instructions that receive the variable location, and that doesn't seem to be the case in the given example. Here's a gist <https://gist.github.com/jmorse/4850a987bfc8d4fed50c622a0b8e7c46> with the input test.cpp at the bottom, and IR for foo.resume on IR line 693 onwards. I've compiled this with a ~6 week old clang master and `-emit-llvm -S -g -O0 -fcoroutines-ts -stdlib=libc++`. I'm not familiar with the coroutine implementation details and assumed:

- foo.resume is the function we care about as it's the first function gdb stops in when I set breakpoints in the "foo" function,
- The await.ready block is the main part of the foo.ready function, and everything else is coroutine plumbing.

>From the entry block, there's a path through the blocks thus:

  resume.entry
  resume.1
  resume.1.landing
  AfterCoroSuspend50
  await.ready

Which reaches the await.ready block without going through any debug intrinsics. The variable locations won't be propagated into the await.ready block because of this path, as there's no information about variable locations on it.  This matches what dwarfdump reports: there's a variable location for _some_ of the function, but not all of it, and not the part that the developer wants to step through.

I'm not familiar with the coroutine implementation details, glad to be corrected. (CC @Orlando @chrisjackson , this is an example of where redesigned intrinsics would definitely need to refer to variables in an arbitrary memory location, ouch).


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

https://reviews.llvm.org/D90772