[PATCH] D40512: [Debugify] Add a pass to test debug info preservation

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: tools/opt/Debugify.cpp:10-11
+//
+// This pass attaches synthetic debug info to everything. It can be used to
+// create targeted tests for debug info preservation.
+//
----------------
MatzeB wrote:
> Could use `/// \file This pass ...` so doxygen picks it up.
Will fix.


================
Comment at: tools/opt/Debugify.cpp:38-40
+  // Skip modules with debug info.
+  if (M.getNamedMetadata("llvm.dbg.cu"))
+    return false;
----------------
MatzeB wrote:
> Maybe write a warning to the user to avoid surprises?
Will fix.


================
Comment at: tools/opt/Debugify.cpp:43
+  DIBuilder DIB(M);
+  auto &Ctx = M.getContext();
+
----------------
MatzeB wrote:
> I'm personally no fan of `auto` if you cannot deduce the type immediately. It's not a deal breaker but I consider it friendlier to readers if types are spelled out as much as possible. With the only exception being `auto X = dyn_cast<xxx>()` as the `xxx` type can be seen in the same line or lambdas where the type is also obvious from the RHS of the assignment.
Will fix.


================
Comment at: tools/opt/Debugify.cpp:51-52
+    if (!DTy) {
+      std::string Name = "ty" + utostr(Size);
+      DTy = DIB.createBasicType(Name, Size, dwarf::DW_ATE_unsigned);
+    }
----------------
MatzeB wrote:
> I don't know the DIB API, but seeing that createBasicType takes a StringRef makes me wonder if it actually takes the string and copies it or whether we will end up with a reference to a temporary std::string here?
The stringref is internalized by MDString::get, and some canonical string is pulled out (possibly after a copy).


================
Comment at: tools/opt/Debugify.cpp:79
+      for (Instruction &I : BB)
+        I.setDebugLoc(DILocation::get(Ctx, NextLine++, 1, SP));
+
----------------
aprantl wrote:
> Sorry for having this idea so late, but: Would it make sense to combine this with the DebugIR pass, so, instead of assigning arbitrary artificial DILocations, the DILocations actually point back to the original instructions in the IR and are perhaps more useful when debugging a pass?
I don't think there's a way to unify the two tools in a way that's simpler than keeping them separate. We need separate code paths to mark-up *.ll files on disk vs. in-memory Modules. Secondly, to implement -check-debugify on DebugIR output, we'd need to do do extra work to store a list of all DILocations in a separate piece of metadata. IMO having a separate code path for DebugIR would be neater and less complicated.


================
Comment at: tools/opt/Debugify.cpp:143-146
+      errs() << "ERROR: Instruction with empty DebugLoc -- ";
+      I.print(errs());
+      errs() << "\n";
+      HasErrors = true;
----------------
MatzeB wrote:
> vsk wrote:
> > davide wrote:
> > > I wonder whether this should be under `DEBUG()`, so that we don't clutter the release builds?
> > I'd rather not do that, since -check-debugify should indicate that the author is interested in messages related to debug info.
> I think simply printing is fine for a pass like this. I would probably print to stdout as this feels more like the "normal" output of this pass rather than an error condition (from the perspective of this pass).
> 
> If you wanted to you could also take a look at this Pass::print() thing we have as sort of an official interface to have a pass print information as part of `opt`. Though I don't know for sure whether it would work as intended here (and I would expect it to be little better than directly printing as is done here).
Printing to stdout sgtm, but the Pass::print() mechanism seems a bit heavyweight given how this is structured.


================
Comment at: tools/opt/Debugify.cpp:174
+
+struct DebugifyPass : public ModulePass {
+  bool runOnModule(Module &M) override { return applyDebugifyMetadata(M); }
----------------
aprantl wrote:
> Doxygen comment explaining the purpose of the pass?
Will fix.


================
Comment at: tools/opt/Debugify.cpp:186
+
+struct CheckDebugifyPass : public ModulePass {
+  bool runOnModule(Module &M) override {
----------------
aprantl wrote:
> comment?
Will fix.


https://reviews.llvm.org/D40512