[PATCH] D99179: [RFC] [Coroutines] Enable printing coroutine frame in debugger if program is compiled with -g

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

I'm mostly happy with this now. Do you think it makes sense to factor out all this code into a new file, so it doesn't get confused



================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:341
+    assert(Iter != FieldAlignMap.end());
+    return Iter->second;
+  }
----------------
What does this function add over `FieldAlignMap.lookup(V)`?


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:861
+  } else {
+    LLVM_DEBUG(dbgs() << "Un resolved Ty: " << *Ty << "\n";);
+    std::stringstream UnresolvedTypeName;
----------------
Unresolved


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:863
+    std::stringstream UnresolvedTypeName;
+    UnresolvedTypeName << Name.str() << "_" << Layout.getTypeSizeInBits(Ty);
+    RetType = Builder.createBasicType(
----------------
I would use https://llvm.org/doxygen/classllvm_1_1raw__string__ostream.html here, since it knows about StringRefs. Name.str() will create another temporary copy that is then copied into the stream buffer.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:873
+
+/// Worked for C++ Program with debug information only.
+///
----------------
How about "Build artificial debug info for C++ coroutine frames to allow users to inspect the contents of the frame directly."?


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:878
+/// the following way:
+/// 1. For all the value in the Frame, we search the use of dbg.decalre to find
+///    the corresponding debug variables for the value. If we can find the
----------------
declare


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:914
+      PromiseDIScope, "__coro_frame_ty", DFile, LineNum, Shape.FrameSize * 8,
+      Shape.FrameAlign.value() * 8, llvm::DINode::FlagZero, nullptr,
+      llvm::DINodeArray());
----------------
I believe it would be best to set the Artificial flag on everything that is inserted by this function, since there is no source code that these types and variables belong to.


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

https://reviews.llvm.org/D99179