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

[Duncan P. N. Exon Smith via llvm-commits]

I have a couple of minor comments below; otherwise this LGTM, as long
as you can somehow prevent the DIBuilder API changes from inserting
bugs in frontends (more below).

> On 2015-Nov-03, at 13:17, Peter Collingbourne <peter at pcc.me.uk> wrote:
> 
> 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.

Great.  Linking to that from the PR (and mentioning where to find it in
your eventual commit message) will help out-of-tree people a lot.


> 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.
> 

Cool, thanks.

> Index: lib/Bitcode/Reader/BitcodeReader.cpp
> ===================================================================
> --- lib/Bitcode/Reader/BitcodeReader.cpp
> +++ lib/Bitcode/Reader/BitcodeReader.cpp
> @@ -5134,6 +5151,11 @@
>      }
>    }
>  
> +  // Finish fn->subprogram upgrade for materialized functions.
> +  auto SP = FunctionsWithSPs.find(F);
> +  if (SP != FunctionsWithSPs.end())
> +    F->setSubprogram(SP->second);
> +

I think this reads a little more clearly as:
--
if (DISubprogram *SP = FunctionsWithSPs.lookup(F))
  F->setSubprogram(SP->second);
--
(assuming that or something like it compiles).

>    // Bring in any functions that this function forward-referenced via
>    // blockaddresses.
>    return materializeForwardReferencedFunctions();
> 

>> I didn't look at tests... did you have an upgrade test?
> 
> 
> Yes, see `test/Bitcode/upgrade-subprogram.ll`.
> 
Yup, this looks great.

Regarding the DIBuilder API:

> Index: include/llvm/IR/DIBuilder.h
> ===================================================================
> --- include/llvm/IR/DIBuilder.h
> +++ include/llvm/IR/DIBuilder.h
> @@ -521,27 +521,25 @@
>                     DIFile *File, unsigned LineNo, DISubroutineType *Ty,
>                     bool isLocalToUnit, bool isDefinition, unsigned ScopeLine,
>                     unsigned Flags = 0, bool isOptimized = false,
> -                   Function *Fn = nullptr, MDNode *TParam = nullptr,
> -                   MDNode *Decl = nullptr);
> +                   MDNode *TParam = nullptr, MDNode *Decl = nullptr);

C++ APIs are awkward to update when they have defaults.  How can we
protect against callers that were explicitly passing `nullptr` for the
`Function*` argument?

Maybe for now you should add overloads such as:
--
void foo(DIFile *File, unsigned LineNo, DISubroutineType *Ty,
         bool isLocalToUnit, bool isDefinition, unsigned ScopeLine,
         unsigned Flags = 0, bool isOptimized = false,
         MDNode *TParam = nullptr, MDNode *Decl = nullptr);
void foo(DIFile *File, unsigned LineNo, DISubroutineType *Ty,
         bool isLocalToUnit, bool isDefinition, unsigned ScopeLine,
         unsigned Flags = 0, bool isOptimized, std::nullptr_t,
         MDNode *TParam, MDNode *Decl = nullptr);
--
??

Alternatively, changing the `Decl` argument to a `DISubprogram*` might
be enough to fix this...

Alternatively, you don't need to change `DIBuilder` *at all* in this
commit.  Instead, you can have `createSubprogram()` call
`F->setSubprogram()`.  This would defer the API change to a separate
commit.  I like this approach the best if it's not too hard.

>  
>      /// Identical to createFunction,
>      /// except that the resulting DbgNode is meant to be RAUWed.
>      DISubprogram *createTempFunctionFwdDecl(
>          DIScope *Scope, StringRef Name, StringRef LinkageName, DIFile *File,
>          unsigned LineNo, DISubroutineType *Ty, bool isLocalToUnit,
>          bool isDefinition, unsigned ScopeLine, unsigned Flags = 0,
> -        bool isOptimized = false, Function *Fn = nullptr,
> -        MDNode *TParam = nullptr, MDNode *Decl = nullptr);
> +        bool isOptimized = false, MDNode *TParam = nullptr,
> +        MDNode *Decl = nullptr);
>  
>      /// FIXME: this is added for dragonegg. Once we update dragonegg
>      /// to call resolve function, this will be removed.
>      DISubprogram *
>      createFunction(DIScopeRef Scope, StringRef Name, StringRef LinkageName,
>                     DIFile *File, unsigned LineNo, DISubroutineType *Ty,
>                     bool isLocalToUnit, bool isDefinition, unsigned ScopeLine,
>                     unsigned Flags = 0, bool isOptimized = false,
> -                   Function *Fn = nullptr, MDNode *TParam = nullptr,
> -                   MDNode *Decl = nullptr);
> +                   MDNode *TParam = nullptr, MDNode *Decl = nullptr);
>  
>      /// Create a new descriptor for the specified C++ method.
>      /// See comments in \a DISubprogram* for descriptions of these fields.
> @@ -568,7 +566,7 @@
>                   bool isLocalToUnit, bool isDefinition, unsigned Virtuality = 0,
>                   unsigned VTableIndex = 0, DIType *VTableHolder = nullptr,
>                   unsigned Flags = 0, bool isOptimized = false,
> -                 Function *Fn = nullptr, MDNode *TParam = nullptr);
> +                 MDNode *TParam = nullptr);
>  
>      /// This creates new descriptor for a namespace with the specified
>      /// parent scope.