r228588 - DebugInfo: Suppress the location of instructions in aggregate default arguments.

[David Blaikie]

On Mon, Feb 9, 2015 at 11:28 AM, Frédéric Riss <friss at apple.com> wrote:

>
> On Feb 9, 2015, at 11:09 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Mon, Feb 9, 2015 at 11:07 AM, Frédéric Riss <friss at apple.com> wrote:
>
>>
>> > On Feb 9, 2015, at 10:47 AM, David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> > Author: dblaikie
>> > Date: Mon Feb  9 12:47:14 2015
>> > New Revision: 228588
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=228588&view=rev
>> > Log:
>> > DebugInfo: Suppress the location of instructions in aggregate default
>> arguments.
>> >
>> > Matches the existing code for scalar default arguments. Complex default
>> > arguments probably need the same handling too (test/fix to that coming
>> > next).
>>
>> What was the behavior before that? Was the location set to the
>> declaration with the default argument description?
>
>
> Yes
>
>
>> I think I wouldn’t matter my debugger jumping there when the code
>> actually does something non-trivial to initialize the argument(s).
>>
>
> Perhaps - though it is a bit jarring whenever you step passed a
> std::string, std::vector, etc ctor and leap into the standard library
> header to see the Allocator() ctor call, then go back to your code.
>
>
> This is true, however from a purely semantic POV I think a ‘step’ should
> really get you there. I mostly asked out of curiosity, I’m not objecting to
> the change. I guess people could find it strange and that hiding the
> initialization is a sane choice (although remembering the developer that
> something non-trivial is happening here also has its advantages).
>

I'm not sure how to remind the developer in a sufficiently gentle way
(perhaps some way we could communicate this to a debugger & come up with an
appropriate debugger user experience) - default arguments are /really/
common in C++. If we attributed their location to where the expression was
written, it would be very disruptive.


>
> I do wonder if maybe the same is_stmt change we're discussing for dtors
> would be suitable here, so you don't step to it, but it does appear in the
> backtrace. Still awkward line table entry "foo() : basic_string:20473" or
> something - "wait, foo isn't in basic_string…".
>
>
> Not sure I’m totally following here. The basic_string code in foo() should
> appear as an inlined function, shouldn’t it?
>

It's not the body of basic_string's ctor, it's basic_string's ctor's
default argument (changed somewhat in C++14, but prior to that there was no
no-args ctor in basic_string, only one that took an Allocator and provided
a default value:
http://en.cppreference.com/w/cpp/string/basic_string/basic_string - but you
can see the other ctors that have the same default argument post-14,
including the common const char* converting ctor)

class basic_string {
  basic_string(const Allocator &alloc = Allocator());
  ...
};

void func() {
  basic_string s;
}

In this code, there is a call to "Allocator()" in the body of func() - this
is not the inlined body of basic_string, it's not an inlined function of
any kind, the C++ language just says, basically, that the default argument
expression is evaluated in the context of the call site (modulo some
stuff). So the compiler generates that call to Allocator() at every call to
basic_string's construction with no args.

So nexting through 'func()' would result in you jumping off to where
"Allocator()" was written, then jumping back to 'func()'.

(you can try commenting out the code - refactored in r228589 - and
debugging some STL code)


>
> Fred
>
>
>> Fred
>>
>> > Modified:
>> >    cfe/trunk/lib/CodeGen/CGExprAgg.cpp
>> >    cfe/trunk/test/CodeGenCXX/debug-info-line.cpp
>> >
>> > Modified: cfe/trunk/lib/CodeGen/CGExprAgg.cpp
>> > URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprAgg.cpp?rev=228588&r1=228587&r2=228588&view=diff
>> >
>> ==============================================================================
>> > --- cfe/trunk/lib/CodeGen/CGExprAgg.cpp (original)
>> > +++ cfe/trunk/lib/CodeGen/CGExprAgg.cpp Mon Feb  9 12:47:14 2015
>> > @@ -1387,7 +1387,12 @@ void CodeGenFunction::EmitAggExpr(const
>> >   // Optimize the slot if possible.
>> >   CheckAggExprForMemSetUse(Slot, E, *this);
>> >
>> > +  bool hasDebugInfo = getDebugInfo();
>> > +  if (isa<CXXDefaultArgExpr>(E))
>> > +    disableDebugInfo();
>> >   AggExprEmitter(*this, Slot).Visit(const_cast<Expr*>(E));
>> > +  if (isa<CXXDefaultArgExpr>(E) && hasDebugInfo)
>> > +    enableDebugInfo();
>> > }
>> >
>> > LValue CodeGenFunction::EmitAggExprToLValue(const Expr *E) {
>> >
>> > Modified: cfe/trunk/test/CodeGenCXX/debug-info-line.cpp
>> > URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/debug-info-line.cpp?rev=228588&r1=228587&r2=228588&view=diff
>> >
>> ==============================================================================
>> > --- cfe/trunk/test/CodeGenCXX/debug-info-line.cpp (original)
>> > +++ cfe/trunk/test/CodeGenCXX/debug-info-line.cpp Mon Feb  9 12:47:14
>> 2015
>> > @@ -274,6 +274,17 @@ void f22() {
>> >   }
>> > }
>> >
>> > +// CHECK-LABEL: define
>> > +struct f23_struct {
>> > +};
>> > +f23_struct f23_a();
>> > +void f23_b(f23_struct = f23_a());
>> > +void f23() {
>> > +// CHECK: call {{.*}}f23_a{{.*}}, !dbg [[DBG_F23:![0-9]*]]
>> > +#line 2500
>> > +  f23_b();
>> > +}
>> > +
>> > // CHECK: [[DBG_F1]] = !MDLocation(line: 100,
>> > // CHECK: [[DBG_FOO_VALUE]] = !MDLocation(line: 200,
>> > // CHECK: [[DBG_FOO_REF]] = !MDLocation(line: 202,
>> > @@ -302,3 +313,4 @@ void f22() {
>> > // CHECK: [[DBG_F19_1]] = !MDLocation(line: 2100,
>> > // CHECK: [[DBG_F19_2]] = !MDLocation(line: 2101,
>> > // CHECK: [[DBG_F20_1]] = !MDLocation(line: 2200,
>> > +// CHECK: [[DBG_F23]] = !MDLocation(line: 2500,
>> >
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150209/a130fadd/attachment.html>