[PATCH] D14265: DI: Reverse direction of subprogram -> function edge.

[Peter Collingbourne via llvm-commits]

pcc added a comment.

Adrian wrote:

> Could you include your upgrade script so people can upgrade their out-of-tree testcases?


F1054572: add-subprogram-attachments.sh <http://reviews.llvm.org/F1054572>
Attached. This is basically the same as Duncan's upgrade script attached to PR23367 with a quick hack to remove function references from subprograms.

Duncan wrote:

> This call is only valid if `F` is a definition.  Otherwise it will assert. IIRC, metadata typically gets parsed *before* function bodies, so I think this could even rarely be valid?


It seems that this check is done in the verifier rather than in `setSubprogram` itself. (This actually makes more sense to me because it may be more convenient for a frontend to create the subprogram before generating code for the function.)

But I've done something along the lines of what you suggested in order to ensure that unmaterialized functions are verifiable. Because we always read function prototypes before metadata, I don't think we need to handle the case where the thing that the SP refers to is not a function.

> I didn't look at tests... did you have an upgrade test?


Yes, see `test/Bitcode/upgrade-subprogram.ll`.


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:1027
@@ -1026,3 +1026,3 @@
   Record.push_back(N->isOptimized());
-  Record.push_back(VE.getMetadataOrNullID(N->getRawFunction()));
+  Record.push_back(0);
   Record.push_back(VE.getMetadataOrNullID(N->getTemplateParams().get()));
----------------
dblaikie wrote:
> Could we use the size of the record to determine whether we're in the old format or the new one? Rather than including an empty field?
We can; done.

================
Comment at: lib/IR/DebugInfoMetadata.cpp:364
@@ -364,8 +363,3 @@
 
-Function *DISubprogram::getFunction() const {
-  // FIXME: Should this be looking through bitcasts?
-  return dyn_cast_or_null<Function>(getFunctionConstant());
-}
-
 bool DISubprogram::describes(const Function *F) const {
   assert(F && "Invalid function");
----------------
dblaikie wrote:
> I wonder if this function is even needed anymore... hopefully we'll just not encounter DISubprograms for a Function that aren't attached to the same Function.
I don't think it is. If I replace this function body with just the pointer comparison the test suite still passes. But this change is pretty large as it is, so I'd prefer to make that change separately.


http://reviews.llvm.org/D14265