+dblaikie 'cause debug info

Reproduces without auto and templates, in C++11

void f1();
constexpr void (*f2)() = f1;
void f3() {
  f2();
}

Continuing to investigate.

Hey Duncan, looks like you added this check in r233389 - any idea why? Looks like it's reasonable/possible that a global variable would be initialized with a global function.

(In reply to comment #3)
> Hey Duncan, looks like you added this check in r233389 - any idea why? Looks
> like it's reasonable/possible that a global variable would be initialized
> with a global function.

I agree, this seems reasonable.  For a moment I wondered if I cargo-culted it from DIGlobalVariable::Verify, but no, it looks like I was just wrong.

Duncan: Are you removing the assert / adding a test?

If this is blocking you(In reply to comment #5)
> Duncan: Are you removing the assert / adding a test?

If this is blocking you, please go ahead.  Otherwise I should get to it next week.

Friendly ping :-)

Duncan: Ping?

Duncan: Ping?

Building ToT now so I can try to reproduce.

--
$ clang -g -x c++ -std=gnu++11 - -S -emit-llvm -o -
invalid global varaible ref
!4 = distinct !DIGlobalVariable(name: "five", scope: !0, file: !5, line: 6, type: !6, isLocal: true, isDefinition: true, variable: i32 ()* @_Z8some_intILi5EEiv)
i32 ()* @_Z8some_intILi5EEiv
Assertion failed: (!V->hasBrokenDebugInfo() && "Module contains invalid debug info"), function doFinalization, file /Users/dexonsmith/data/llvm/staging/lib/IR/Verifier.cpp, line 4435.
--

Had the fix (hadn't actually built against ToT before, used a clang from the weekend).  Tried to rebase, and then hit a conflict with r281284.  I'm not sure what happens now for this testcase (rebuilding at real ToT), but DIGlobalVariable will certainly never reference a Function...

+pcc: can an llvm::Function have multiple !dbg attachments?  Can one of them be a DIGlobalVariable?

Created attachment 17273 [details]
patch that applied before pcc changed the world in r281284

Attaching the patch I was going to test for posterity as pr29122-before-r281284.patch.

(In reply to comment #12)
> Had the fix (hadn't actually built against ToT before, used a clang from the
> weekend).  Tried to rebase, and then hit a conflict with r281284.  I'm not
> sure what happens now for this testcase (rebuilding at real ToT), but
> DIGlobalVariable will certainly never reference a Function...

Confirmed that this was inadvertently fixed by r281284.

> +pcc: can an llvm::Function have multiple !dbg attachments?  Can one of them
> be a DIGlobalVariable?

Confirmed that the link from the Function to the DIGlobalVariable does not exist (gets dropped the floor).  Not surprising, given that there was no coverage (and the Verifier failed).

Adrian, David, etc.; I'll leave it for you to sort out whether this is an important regression.

This is a regression on the LLVM IR side, but also we never produced useful DWARF for this example (the variable never had DW_AT_location or other constant value).

I see two interesting design problems here:

1. How to represent a function constant in IR?
   For function definitions, we could add a !dbg attachment to the function.
   define void @_Z1f3v() #0 !dbg !1, !dbg2 {
     ...
   }

   !1 = distinct !DIGlobalVariable(name: "f2", type: <FnPtr>, ..., expr: ???)
   !2 = !DISubroutineType(types: !3)

   But what to do if this is a forward declaration like f1 in David's example?
   Should we allow symbols to appear in DIExpressions, at the risk that they
   might get optimized away?

   How do we encode that the constant value is the function's address
   (as opposed to the data stored at the address like with global variables)?
   

2. How to represent a function constant in DWARF?
   What if the target is a Harvard architecture with separate
   address spaces for code and data?

(In reply to comment #15)
> 2. How to represent a function constant in DWARF?
>    What if the target is a Harvard architecture with separate
>    address spaces for code and data?

Regarding constexpr functions in general, DWARF 4 section 3.3.8.2
says you give the concrete inlined instance a DW_AT_const_expr flag
and a DW_AT_const_value describing the value returned by the instance.
It's a concrete inlined instance because a 'constexpr' function/method
is implicitly 'inline'.  There's an example in appendix D.8.

A variable that is a function pointer would normally have a location,
which is the storage for the variable, which would contain the address
of the function it points to.
A constexpr variable would instead have a DW_AT_const_value describing
the constant address of the function it points to (and not have a
DW_AT_location attribute).

A constexpr variable pointing to a constexpr function that is in fact
evaluated at compile time... uh.

Maybe give the variable the correct function-pointer type, and the
DW_AT_const_expr flag, and then use DW_AT_location to provide an
expression that calls a DIE describing the concrete inlined instance
(which in turn would be DW_AT_const_expr with DW_AT_const_value).
This is slightly perverting DW_AT_location, I'd like to get the
opinion of somebody who has worked on a debugger more recently than
I have.

A less faithful DWARF description would give the variable the result
type of the function (not a pointer-to-function type) and use 
DW_AT_const_value to describe that value.  This elides the function 
from the debug info entirely, and so is not very satisfactory in terms 
of describing the source (although it would correctly describe the
*translation* of the source).

In either case I don't think Harvard architectures would be special.
In one case the variable is still defined in terms of the function
(which just happens not to have an actual object address) and in the
other case you've eliminated the function from the DWARF entirely.

(In reply to comment #16)
> (In reply to comment #15)
> > 2. How to represent a function constant in DWARF?
> >    What if the target is a Harvard architecture with separate
> >    address spaces for code and data?
> 
> Regarding constexpr functions in general, DWARF 4 section 3.3.8.2
> says you give the concrete inlined instance a DW_AT_const_expr flag
> and a DW_AT_const_value describing the value returned by the instance.
> It's a concrete inlined instance because a 'constexpr' function/method
> is implicitly 'inline'.  There's an example in appendix D.8.

For now I don't think we're worried about that - we wouldn't describe this inlining at all (since there are no instructions associated with the inlining). One day, perhaps, we'll have a sufficiently expressive representation to explain this frontend constant evaluation to the backend to produce an inlined subroutine over zero instructions.

> A variable that is a function pointer would normally have a location,
> which is the storage for the variable, which would contain the address
> of the function it points to.
> A constexpr variable would instead have a DW_AT_const_value describing
> the constant address of the function it points to (and not have a
> DW_AT_location attribute).

A constexpr variable just means it's guaranteed to be initialized at compile time (not link time - so there's no initialization order fiasco), the variable can still have storage (you can take the variable's address, pass it somewhere, etc). But in some cases the storage may be optimized away, leaving only the value and you'd want to use a DW_AT_const_value as you mentioned.

> A constexpr variable pointing to a constexpr function that is in fact
> evaluated at compile time... uh.

At that point I'd expect we would, ideally (again, where nowhere near doing any of this today) describe the function that was actually called as inlined with a const_value/constexpr.

> Maybe give the variable the correct function-pointer type, and the
> DW_AT_const_expr flag, and then use DW_AT_location to provide an
> expression that calls a DIE describing the concrete inlined instance
> (which in turn would be DW_AT_const_expr with DW_AT_const_value).

I'm not sure I follow. If we're giving any value for the global constexpr variable, it should be for the function it points to, or nothing if the function's been optimized away. If the function's been fully inlined (ie: has no concrete out of line instance) then I think we just leave it at that and provide no value for the global variable. I suppose in theory we could provide a DWARF expression that could evaluate to the value of the function call (& put that expression in the DW_AT_const_value attribute) - but how would we access the function's parameters from that DWARF expression? (can we actually map between those domains at all) Again - /far/ off future stuff.

The function may take parameters - so pointing to a concrete inlined instance wouldn't be suitable (since each concrete inlined instance may produce a different value because it may be given different arguments).

> This is slightly perverting DW_AT_location, I'd like to get the
> opinion of somebody who has worked on a debugger more recently than
> I have.
> 
> A less faithful DWARF description would give the variable the result
> type of the function (not a pointer-to-function type) and use 
> DW_AT_const_value to describe that value.  This elides the function 
> from the debug info entirely, and so is not very satisfactory in terms 
> of describing the source (although it would correctly describe the
> *translation* of the source).
> 
> In either case I don't think Harvard architectures would be special.
> In one case the variable is still defined in terms of the function
> (which just happens not to have an actual object address) and in the
> other case you've eliminated the function from the DWARF entirely.

(In reply to comment #17)

I was trying to arrive at a DWARF construct that is able to
- describe a constexpr pointer to a constexpr function
- where the constexpr function is evaluated at compile time, and
- that allows the debugger to evaluate an expression calling the
  function through that pointer. ("five()" in the original example.)

Whether Clang is able to get there from here is a separate question,
but it seems like a prerequisite would be knowing where 'there' is.

Situations where the constexpr function is *not* evaluated at compile
time would behave more like normal function-pointer cases and didn't
seem worth spending time on.

(Also, in my admittedly aging copy of C++11, 'constexpr' does not 
guarantee compile-time initialization (variable) or evaluation (function).
On a variable it's static-initialization which is required to occur prior 
to all dynamic-initialization, which might or might not be compile-time.
Maybe this has changed in subsequent revisions of C++11, but I remember 
being in a lecture at CPPcon that carefully made that distinction.)

(In reply to comment #18)
> (In reply to comment #17)
> 
> I was trying to arrive at a DWARF construct that is able to
> - describe a constexpr pointer to a constexpr function
> - where the constexpr function is evaluated at compile time, and
> - that allows the debugger to evaluate an expression calling the
>   function through that pointer. ("five()" in the original example.)
> 
> Whether Clang is able to get there from here is a separate question,
> but it seems like a prerequisite would be knowing where 'there' is.

Fair - with that in mind, I don't think your description accounts for the case where a constexpr function has parameters:

constexpr int add(int X, int Y) {
  return X + Y;
}

Even before we get to the function pointer case - if we inline all calls to this function, I'm not sure how we'd describe it so a user could call it/do useful things with it. This is the state of things even before we get to constexpr (we can still inline all calls away to a non-constexpr function).

Any /specific/ call that was inlined into a constant could have a DW_AT_const_value with the value of evaluating the constexpr function call, but this doesn't help the user invoke it for other arguments (or even for the arguments used in the inlined instances - since the arguments wouldn't necessarily be encoded in any way the debugger could retrieve and thus map to the user's expression in the debugger)

So without a solution to that, I'm not sure there's anything we can do for the function pointer to such a function.

> 
> Situations where the constexpr function is *not* evaluated at compile
> time would behave more like normal function-pointer cases and didn't
> seem worth spending time on.
> 
> (Also, in my admittedly aging copy of C++11, 'constexpr' does not 
> guarantee compile-time initialization (variable) or evaluation (function).
> On a variable it's static-initialization which is required to occur prior 
> to all dynamic-initialization, which might or might not be compile-time.
> Maybe this has changed in subsequent revisions of C++11, but I remember 
> being in a lecture at CPPcon that carefully made that distinction.)

Oh, fair enough - good point. Yes s/compile-time/static-initialization/ in my prior statements. Equivalent for our purposes, perhaps.

(In reply to comment #19)
> Fair - with that in mind, I don't think your description accounts for the
> case where a constexpr function has parameters:
> 
> constexpr int add(int X, int Y) {
>   return X + Y;
> }
> 
> Even before we get to the function pointer case - if we inline all calls to
> this function, I'm not sure how we'd describe it so a user could call it/do
> useful things with it. This is the state of things even before we get to
> constexpr (we can still inline all calls away to a non-constexpr function).

Ah, true, the original example didn't have parameters and that may
have led me down the garden path.  Probably not worth making a special
case for the zero-parameters case.

So, the constexpr pointer-to-function would try to point to an instance
if there is one, which might disappear because that happens sometimes.
And if the pointed-to function is constexpr, probably happens a lot.
Oh well.

So you don't get to evaluate "five()" in the debugger after all.

If doing the Right Thing is difficult (or maybe not even expressible in dwarf), can we do something Not Right That At Least Doesn't Crash in the meantime?

Is this still an issue or something that got fixed in the meantime?

pcc fixed it in r281284.