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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.

LGTM to go in tree. Comments/nitpicks below.



================
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.
+//
----------------
Could use `/// \file This pass ...` so doxygen picks it up.


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


================
Comment at: tools/opt/Debugify.cpp:43
+  DIBuilder DIB(M);
+  auto &Ctx = M.getContext();
+
----------------
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.


================
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);
+    }
----------------
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?


================
Comment at: tools/opt/Debugify.cpp:143-146
+      errs() << "ERROR: Instruction with empty DebugLoc -- ";
+      I.print(errs());
+      errs() << "\n";
+      HasErrors = true;
----------------
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).


https://reviews.llvm.org/D40512