llvm dev - Dec 2014 - [LLVMdev] Metadata/Value split has landed

> On 2014-Dec-18, at 01:43, Keno Fischer <kfischer at
college.harvard.edu> wrote:
> 
> Hi Duncan,
> 
> I'm in the following situation for which this change caused an
assertion failure:
> 
> Three modules, let's say A B C
> Two function, F in A, G in B
> 
> Now I CloneFunction F into B (call the new function F') and inline
F' into G.
> Now, for the problematic part, where I try to extract G (and all referenced
values) into C:
> 
> upon encountering any debug node in the inlined code, it tries to clone the
DISubprogram for F', so it creates a temporary. Since that refers to F',
it'll now go ahead and copy F'. However, here once again it tries to
copy the DISubprogram, which now just uses the temporary value from above (this
is fine). Unfortunately, right after, it calls resolveCycles on the debug info
annotation, which crashes with
> 
> Assertion failed: (!isa<MDNodeFwdDecl>(Op) && "Expected
all forward declarations to be resolved"), function resolveCycles, file
/Users/kfischer/julia/deps/llvm-svn/lib/IR/Metadata.cpp, line 459.
> 
> because we still have the temporary DISubprogram in there. 
> 
> Any ideas what to do about this?
> 
The logic I have between `MapValue(Metadata*)` and `MapValueImpl(Metadata*)`
was supposed to solve this.  (Actually, those names are awful, I may try to
change them to `MapMetadata()`.)

The logical flow should be:

  - MapValue(Metadata*) calls MapValueImpl().
  - MapValueImpl() introduces a temporary.
  - MapValueImpl() recursively calls MapValueImpl().
  - MapValueImpl() resolves its temporary.
  - MapValue(Metadata*) calls resolveCycles().

There could be a bug here, but I just scanned the code and it seems to
match up.  I suspect, rather, that there's a temporary node in the metadata
graph on entry to `CloneFunction()`; this isn't supported, and would cause
the same assertion to fire.

Try running the verifier right before calling `CloneFunction()`: I put a
check for this in `Verifier::visitMDNode()`.

> On 2014-Dec-18, at 10:00, Duncan P. N. Exon Smith <dexonsmith at
apple.com> wrote:
> 
> 
>> On 2014-Dec-18, at 01:43, Keno Fischer <kfischer at
college.harvard.edu> wrote:
>> 
>> Hi Duncan,
>> 
>> I'm in the following situation for which this change caused an
assertion failure:
>> 
>> Three modules, let's say A B C
>> Two function, F in A, G in B
>> 
>> Now I CloneFunction F into B (call the new function F') and inline
F' into G.
>> Now, for the problematic part, where I try to extract G (and all
referenced values) into C:
>> 
>> upon encountering any debug node in the inlined code, it tries to clone
the DISubprogram for F', so it creates a temporary. Since that refers to
F', it'll now go ahead and copy F'. However, here once again it
tries to copy the DISubprogram, which now just uses the temporary value from
above (this is fine). Unfortunately, right after, it calls resolveCycles on the
debug info annotation, which crashes with
>> 
>> Assertion failed: (!isa<MDNodeFwdDecl>(Op) &&
"Expected all forward declarations to be resolved"), function
resolveCycles, file /Users/kfischer/julia/deps/llvm-svn/lib/IR/Metadata.cpp,
line 459.
>> 
>> because we still have the temporary DISubprogram in there. 
>> 
>> Any ideas what to do about this?
>> 
> 
> The logic I have between `MapValue(Metadata*)` and
`MapValueImpl(Metadata*)`
> was supposed to solve this.  (Actually, those names are awful, I may try to
> change them to `MapMetadata()`.)
> 
> The logical flow should be:
> 
>  - MapValue(Metadata*) calls MapValueImpl().
>  - MapValueImpl() introduces a temporary.
>  - MapValueImpl() recursively calls MapValueImpl().
>  - MapValueImpl() resolves its temporary.
>  - MapValue(Metadata*) calls resolveCycles().
> 
> There could be a bug here, but I just scanned the code and it seems to
> match up.  I suspect, rather, that there's a temporary node in the
metadata
> graph on entry to `CloneFunction()`; this isn't supported, and would
cause
> the same assertion to fire.
> 
> Try running the verifier right before calling `CloneFunction()`: I put a
> check for this in `Verifier::visitMDNode()`.
Or maybe you're not using `MapValue()`, and doing your own thing?  If
that's
the case, you just need to do something similar with your logic!