User account creation filtered due to spam.

Bug 59821 - __builtin_LINE and __builtin_FILE for new'd objects is wrong
Summary: __builtin_LINE and __builtin_FILE for new'd objects is wrong
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.8.2
: P3 normal
Target Milestone: 4.9.0
Assignee: Jason Merrill
URL:
Keywords: wrong-debug
Depends on:
Blocks:
 
Reported: 2014-01-15 08:35 UTC by Jörg Richter
Modified: 2014-01-17 12:41 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2014-01-15 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jörg Richter 2014-01-15 08:35:20 UTC
The script below demonstrates that __builtin_LINE used in constructors
is different when the object is allocated with new.  The same is true for
__builtin_FILE and I suspect also for __builtin_FUNCTION.


cat > t.cc <<EOF
#include <cassert>
struct Foo
{
    int line;
    Foo( int line = __builtin_LINE() )
      : line( line )
    {}
};

int main()
{
  assert( Foo().line == (new Foo)->line );
}
EOF

g++ -o t t.cc
./t
Comment 1 Richard Biener 2014-01-15 11:47:23 UTC
    [t.C : 12:3] Foo::Foo ([t.C : 12] &D.2242, 12);
    [t.C : 12:42] try
      {
        [t.C : 12:3] D.2247 = D.2242.line;
        [t.C : 12:3] D.2244 = 5;
        [t.C : 12:3] D.2243 = operator new (4);
        [t.C : 12:3] try
          {
            [t.C : 12:3] Foo::Foo (D.2243, D.2244);

looks like location info is broken for the new case.
Comment 2 Jakub Jelinek 2014-01-15 16:33:32 UTC
For the calls this just happens to work completely by accident, nothing else.
I mean, the default argument expression obviously has location_t from where it has been defined, i.e. the function/method/ctor definition.  The reason why in the first case you get the locus of the call is that gimplify_arg sets the location_t of the arguments to that, but only on the toplevel expression.
Now, in the new case the C++ FE adds an TARGET_EXPR around the default argument and so during the gimplification it is no longer toplevel expression in the argument, so gimplify_arg doesn't adjust it.

If you write:
struct Foo
{
  int line;
  Foo (int line = __builtin_LINE () + 1) : line (line) {}
};
int
main ()
{
  if (Foo().line != (new Foo)->line)
    __builtin_abort ();
}

then it will be in both cases the location of the Foo ctor, because the argument will not be __builtin_LINE() CALL_EXPR as toplevel.

So, IMHO if you want a different behavior, you shouldn't leave this to the gimplifier, but explicitly change it in the FE.  Say in convert_default_arg or functions it calls change EXPR_LOCATION of all CALL_EXPRs to the 3 problematic builtins (I'd argue that only those and nothing else, the expressions really have locus of where the default argument has been defined and it is desirable to use that for diagnostic purposes etc.).  convert_default_arg already walks the expression several times through break_out_target_exprs, so if we e.g. wanted to
avoid the cost of walking it once again, break_out_target_exprs could have a flag to tweak location_t of __builtin_{LINE,FILE,FUNCTION}.  Jason, thoughts on this?  In any case this needs to be documented.  What about other default locations?  Say if e.g. NSDMI contains __builtin_LINE () call, do we want to get a value of where the NSDMI is defined, or where the object is constructed?
Comment 3 Jason Merrill 2014-01-16 17:06:46 UTC
I think gimplify_arg clobbering a valid EXPR_LOCATION is wrong; both cases should give the location of the default argument definition.

Aldy, what was the rationale for that change (r140917)?
Comment 4 Jakub Jelinek 2014-01-16 17:10:12 UTC
While I agree with that, for _builtin_{LINE,FILE,FUNCTION} (), those were added specifically for the use in default arguments and were I think from the start meant to give you the location of the call, not of the token where it appears,
otherwise those builtins don't make much sense (you could use __LINE__, __FILE__
or __FUNCTION__ instead).
Comment 5 Aldy Hernandez 2014-01-16 18:02:07 UTC
(In reply to Jason Merrill from comment #3)
> I think gimplify_arg clobbering a valid EXPR_LOCATION is wrong; both cases
> should give the location of the default argument definition.
> 
> Aldy, what was the rationale for that change (r140917)?

Unfortunately this was by design to make multi-line non-trivial arguments play nice with the debugger.  Perhaps it's time to revisit the ``is_stmt'' DWARF flag for this?

More details here:

http://gcc.gnu.org/ml/gcc-patches/2008-10/msg00191.html
Comment 6 Jason Merrill 2014-01-16 18:18:13 UTC
(In reply to Jakub Jelinek from comment #4)
> While I agree with that, for _builtin_{LINE,FILE,FUNCTION} (), those were
> added specifically for the use in default arguments and were I think from
> the start meant to give you the location of the call, not of the token where
> it appears, otherwise those builtins don't make much sense (you could use 
> __LINE__, __FILE__ or __FUNCTION__ instead).

Ah, I see.  In that case, I agree that adjusting their locations in break_out_target_exprs is the way to go.
Comment 7 Jason Merrill 2014-01-16 19:55:43 UTC
Author: jason
Date: Thu Jan 16 19:55:12 2014
New Revision: 206686

URL: http://gcc.gnu.org/viewcvs?rev=206686&root=gcc&view=rev
Log:
	PR c++/59821
	* tree.c (bot_manip): Update the location of builtin_LINE and
	builtin_FILE calls.

Added:
    trunk/gcc/testsuite/g++.dg/ext/builtin-line1.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/tree.c
    trunk/gcc/doc/extend.texi
Comment 8 Jason Merrill 2014-01-16 19:57:36 UTC
Fixed for 4.9.
Comment 9 Richard Biener 2014-01-17 12:41:18 UTC
Yeah - sorry for this.  As it "worked" for the testcases I tested I was assuming
that the locations for the default argument computation (!) stmts should be
that of the call, not that of the definition.  Appearantly that isn't so.

Thanks for fixing it.