Don't set DECL_IGNORED_P on a cdtor function

Don't set DECL_IGNORED_P on a cdtor function

[Geoffrey Keating]

DECL_IGNORED_P shouldn't be set on any function which the user might
want to set a breakpoint in, or which might appear in a stack
backtrace, or which might crash.  This is especially bad for
backtraces, because DECL_IGNORED_P means that there's no DWARF frame
information, which might mean that no further backtracing is possible.

Bootstrapped & tested on powerpc-darwin8.

-- 
- Geoffrey Keating <geoffk@apple.com>

===File ~/patches/gcc-noignoreconstructor.patch=============
2007-10-04  Geoffrey Keating  <geoffk@apple.com>

	* cgraphunit.c (cgraph_build_static_cdtor): Don't set
	DECL_IGNORED_P.

Index: gcc/cgraphunit.c
===================================================================
--- gcc/cgraphunit.c	(revision 128879)
+++ gcc/cgraphunit.c	(working copy)
@@ -1400,7 +1400,6 @@
 
   resdecl = build_decl (RESULT_DECL, NULL_TREE, void_type_node);
   DECL_ARTIFICIAL (resdecl) = 1;
-  DECL_IGNORED_P (resdecl) = 1;
   DECL_RESULT (decl) = resdecl;
 
   allocate_struct_function (decl);
@@ -1408,7 +1407,6 @@
   TREE_STATIC (decl) = 1;
   TREE_USED (decl) = 1;
   DECL_ARTIFICIAL (decl) = 1;
-  DECL_IGNORED_P (decl) = 1;
   DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT (decl) = 1;
   DECL_SAVED_TREE (decl) = body;
   TREE_PUBLIC (decl) = ! targetm.have_ctors_dtors;
============================================================

Re: Don't set DECL_IGNORED_P on a cdtor function

[Daniel Jacobowitz]

On Thu, Oct 04, 2007 at 02:23:17PM -0700, Geoffrey Keating wrote:
> 
> DECL_IGNORED_P shouldn't be set on any function which the user might
> want to set a breakpoint in, or which might appear in a stack
> backtrace, or which might crash.  This is especially bad for
> backtraces, because DECL_IGNORED_P means that there's no DWARF frame
> information, which might mean that no further backtracing is possible.

The lack of DWARF frame information sounds like the real bug.  Perhaps
in Eric's February changes on how DECL_IGNORED_P was handled for
functions?

I don't think there should be .debug_info for these functions.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Don't set DECL_IGNORED_P on a cdtor function

[Eric Botcazou]

> The lack of DWARF frame information sounds like the real bug.  Perhaps
> in Eric's February changes on how DECL_IGNORED_P was handled for
> functions?

If the user is supposed to able to set a breakpoint in the function, I don't 
think that the function should have DECL_IGNORED_P on it.

-- 
Eric Botcazou

Re: Don't set DECL_IGNORED_P on a cdtor function

[Daniel Jacobowitz]

On Fri, Oct 05, 2007 at 12:00:32AM +0200, Eric Botcazou wrote:
> > The lack of DWARF frame information sounds like the real bug.  Perhaps
> > in Eric's February changes on how DECL_IGNORED_P was handled for
> > functions?
> 
> If the user is supposed to able to set a breakpoint in the function, I don't 
> think that the function should have DECL_IGNORED_P on it.

It doesn't correspond to any source... well, you could argue it either
way.  Anyway, shouldn't we output frame information regardless of DECL_IGNORED_P?

-- 
Daniel Jacobowitz
CodeSourcery

Re: Don't set DECL_IGNORED_P on a cdtor function

[Eric Botcazou]

> It doesn't correspond to any source...

/* Used to indicate that this DECL represents a compiler-generated entity.  */
#define DECL_ARTIFICIAL(NODE) (DECL_COMMON_CHECK 
(NODE)->decl_common.artificial_flag)

> Anyway, shouldn't we output frame information regardless of DECL_IGNORED_P?

Why?

-- 
Eric Botcazou

Re: Don't set DECL_IGNORED_P on a cdtor function

[Geoffrey Keating]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]


On 04/10/2007, at 2:35 PM, Daniel Jacobowitz wrote:

> On Thu, Oct 04, 2007 at 02:23:17PM -0700, Geoffrey Keating wrote:
>>
>> DECL_IGNORED_P shouldn't be set on any function which the user might
>> want to set a breakpoint in, or which might appear in a stack
>> backtrace, or which might crash.  This is especially bad for
>> backtraces, because DECL_IGNORED_P means that there's no DWARF frame
>> information, which might mean that no further backtracing is  
>> possible.

> I don't think there should be .debug_info for these functions.

Why not?

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2462 bytes --]

Re: Don't set DECL_IGNORED_P on a cdtor function

[Daniel Jacobowitz]

On Thu, Oct 04, 2007 at 03:13:44PM -0700, Geoffrey Keating wrote:
> Why not?

It seems useless.  We miss enough useful debug info to generate more
useless info.  It makes me wonder why we support not generating debug
info for anything... But I won't argue about it any more.

I still think there is a problem with omitting .debug_frame though;
that's more important.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Don't set DECL_IGNORED_P on a cdtor function

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 395 bytes --]

> I still think there is a problem with omitting .debug_frame though;
> that's more important.

I've attached an Ada testcase, which compiles down into 2 functions, the 
second one being DECL_IGNORED_P.  AFAICS .debug_frame contains 2 FDEs.
Is this what you meant by "frame information regardless of DECL_IGNORED_P"?

That being said, things might work differently on Darwin.

-- 
Eric Botcazou

[-- Attachment #2: p.adb --]
[-- Type: text/x-adasrc, Size: 512 bytes --]

with Ada.Text_IO; use Ada.Text_IO;

procedure P is

    type Operation_Mode is
      (NonInitializeMode,
       AutomaticMode,
       SetupMode,
       SelectOnlyMode,
       ServiceModeLet,
       ResetMode);

    ModOpn : Operation_Mode := NonInitializeMode;

begin
    loop
       if ModOpn = Operation_Mode'Last then
          ModOpn := Operation_Mode'First;
       else
          ModOpn := Operation_Mode'Succ (ModOpn);
       end if;
       Put_Line (ModOpn'Img);  -- LINE 22 is here
     end loop;
end P;

Re: Don't set DECL_IGNORED_P on a cdtor function

[Eric Botcazou]

> That being said, things might work differently on Darwin.

They apparently don't, both FDEs are there on Darwin.

But on VxWorks, the second FDE is indeed missing, because the target is 
DWARF2_DEBUGGING_INFO but not DWARF2_UNWIND_INFO.  So, if we want to emit
an unchanged .debug_frame, my February change should probably be reverted.

Note that .debug_frame is hampered on VxWorks for a different reason:
  http://gcc.gnu.org/ml/gcc-patches/2007-09/msg01329.html
so it would be nice to have all this ironed out simultaneously.

-- 
Eric Botcazou

Re: Don't set DECL_IGNORED_P on a cdtor function

[Daniel Jacobowitz]

On Fri, Oct 05, 2007 at 09:35:18AM +0200, Eric Botcazou wrote:
> > I still think there is a problem with omitting .debug_frame though;
> > that's more important.
> 
> I've attached an Ada testcase, which compiles down into 2 functions, the 
> second one being DECL_IGNORED_P.  AFAICS .debug_frame contains 2 FDEs.
> Is this what you meant by "frame information regardless of DECL_IGNORED_P"?
> 
> That being said, things might work differently on Darwin.

I'm just going by what Geoff said:

  DECL_IGNORED_P means that there's no DWARF frame information

I unfortunately don't have an Ada compiler newer than your change, or
a Darwin system, so I can't really check what's going on.  Maybe Geoff
was checking against a different tree which had a problem similar to
VxWorks?

-- 
Daniel Jacobowitz
CodeSourcery

Re: Don't set DECL_IGNORED_P on a cdtor function

[Andrew Haley]

Daniel Jacobowitz writes:
 > On Fri, Oct 05, 2007 at 09:35:18AM +0200, Eric Botcazou wrote:
 > > > I still think there is a problem with omitting .debug_frame though;
 > > > that's more important.
 > > 
 > > I've attached an Ada testcase, which compiles down into 2 functions, the 
 > > second one being DECL_IGNORED_P.  AFAICS .debug_frame contains 2 FDEs.
 > > Is this what you meant by "frame information regardless of DECL_IGNORED_P"?
 > > 
 > > That being said, things might work differently on Darwin.
 > 
 > I'm just going by what Geoff said:
 > 
 >   DECL_IGNORED_P means that there's no DWARF frame information

I'm getting confused here.  I can understand that DECL_IGNORED_P
prevents the emission of debugging frame information, but I assume
that it doesn't prevent the emission of unwinder frame information.

Andrew.

Re: Don't set DECL_IGNORED_P on a cdtor function

[Daniel Jacobowitz]

On Fri, Oct 05, 2007 at 03:52:38PM +0100, Andrew Haley wrote:
>  > I'm just going by what Geoff said:
>  > 
>  >   DECL_IGNORED_P means that there's no DWARF frame information
> 
> I'm getting confused here.  I can understand that DECL_IGNORED_P
> prevents the emission of debugging frame information, but I assume
> that it doesn't prevent the emission of unwinder frame information.

There's two sets of frame information on targets which use DWARF-2
for exception handling: .debug_frame and .eh_frame.  I don't think
DECL_IGNORED_P should suppress either.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Don't set DECL_IGNORED_P on a cdtor function

[Eric Botcazou]

> There's two sets of frame information on targets which use DWARF-2
> for exception handling: .debug_frame and .eh_frame.  I don't think
> DECL_IGNORED_P should suppress either.

OK, the bottom line is that it doesn't suppress either when they both exist, 
e.g. for languages with EH and for targets with DWARF-2 unwind info.  It
suppresses the former for targets without DWARF-2 unwind info (VxWorks) or
for languages without EH.

It all boils down to which functions you put DECL_IGNORED_P on.  In Ada, we
really want to completely ignore them so .debug_frame is not a problem.

However, given the awkward variability of its effects depending on the target, 
I agree that it's probably better to revert the change.  Will do shortly.

-- 
Eric Botcazou

Re: Don't set DECL_IGNORED_P on a cdtor function

[Eric Botcazou]

> However, given the awkward variability of its effects depending on the
> target, I agree that it's probably better to revert the change.  Will do
> shortly.

Done.  I've committed http://gcc.gnu.org/ml/gcc-patches/2007-09/msg01329.html
as obvious too.

-- 
Eric Botcazou

Re: Don't set DECL_IGNORED_P on a cdtor function

[Mark Mitchell]

Eric Botcazou wrote:
>> However, given the awkward variability of its effects depending on the
>> target, I agree that it's probably better to revert the change.  Will do
>> shortly.
> 
> Done. 

So, I understand correctly, we're now going to again emit .debug_frame
for DECL_IGNORED_P functions.  That seems good to me, as it helps us
display unwind information through such functions.  I think that's a
strong argument that we should always emit .debug_frame, independent of
whether a function is ignored/artificial.

Geoff's original comment was:

> DECL_IGNORED_P shouldn't be set on any function which the user might
> want to set a breakpoint in, or which might appear in a stack
> backtrace, or which might crash.

There are 3 parts to the comment:

1. Setting breakpoints
2. Backtraces
3. Crashing

Eric's patch solves (2).

I don't think (1) or (3) are sufficient reasons to avoid setting
DECL_IGNORED_P.  Almost all functions might crash (e.g., by extending
the stack beyond the allocated maximum stack size) and *some* users
might want to set breakpoints anywhere.  (Of course, they can always set
a breakpoint by PC or by object-file symbol name anyhow.)

The key issues are that (a) we have no source for these
compiler-generated functions, and (b) they don't correspond to anything
in the source program.  If I say "step" off the end of a C++ "main", I
don't want to end up in one of these functions; I want to end up in the
first C++ destructor being called by them.  That's the C++ view of the
world; these cdtor functions are just implementation details.

So, I think we should consider the issue closed, with Eric's patch.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Don't set DECL_IGNORED_P on a cdtor function

[Eric Botcazou]

> So, I understand correctly, we're now going to again emit .debug_frame
> for DECL_IGNORED_P functions.  That seems good to me, as it helps us
> display unwind information through such functions.  I think that's a
> strong argument that we should always emit .debug_frame, independent of
> whether a function is ignored/artificial.

Agreed.

> There are 3 parts to the comment:
>
> 1. Setting breakpoints
> 2. Backtraces
> 3. Crashing
>
> Eric's patch solves (2).
>
> I don't think (1) or (3) are sufficient reasons to avoid setting
> DECL_IGNORED_P.  Almost all functions might crash (e.g., by extending
> the stack beyond the allocated maximum stack size) and *some* users
> might want to set breakpoints anywhere.  (Of course, they can always set
> a breakpoint by PC or by object-file symbol name anyhow.)
>
> The key issues are that (a) we have no source for these
> compiler-generated functions, and (b) they don't correspond to anything
> in the source program.  If I say "step" off the end of a C++ "main", I
> don't want to end up in one of these functions; I want to end up in the
> first C++ destructor being called by them.  That's the C++ view of the
> world; these cdtor functions are just implementation details.
>
> So, I think we should consider the issue closed, with Eric's patch.

Your comment seems to imply that Geoff's patch should be reverted too. :-)

-- 
Eric Botcazou

Re: Don't set DECL_IGNORED_P on a cdtor function

[Mark Mitchell]

Eric Botcazou wrote:

>> The key issues are that (a) we have no source for these
>> compiler-generated functions, and (b) they don't correspond to anything
>> in the source program.  If I say "step" off the end of a C++ "main", I
>> don't want to end up in one of these functions; I want to end up in the
>> first C++ destructor being called by them.  That's the C++ view of the
>> world; these cdtor functions are just implementation details.
>>
>> So, I think we should consider the issue closed, with Eric's patch.
> 
> Your comment seems to imply that Geoff's patch should be reverted too. :-)

Yes, I do.  (I hadn't realized it had been committed.)

Does anyone concur?  Object?

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Don't set DECL_IGNORED_P on a cdtor function

[Geoffrey Keating]

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]


On 08/10/2007, at 5:35 PM, Mark Mitchell wrote:

> Eric Botcazou wrote:
>
>>> The key issues are that (a) we have no source for these
>>> compiler-generated functions, and (b) they don't correspond to  
>>> anything
>>> in the source program.  If I say "step" off the end of a C++  
>>> "main", I
>>> don't want to end up in one of these functions; I want to end up  
>>> in the
>>> first C++ destructor being called by them.  That's the C++ view  
>>> of the
>>> world; these cdtor functions are just implementation details.
>>>
>>> So, I think we should consider the issue closed, with Eric's patch.
>>
>> Your comment seems to imply that Geoff's patch should be reverted  
>> too. :-)
>
> Yes, I do.  (I hadn't realized it had been committed.)
>
> Does anyone concur?  Object?

The entire paragraph under 'Eric Botcazou wrote' lists reasons why my  
patch is correct.

"(a)": well, if you don't have source, tell the debugger that, rather  
than having the debugger associate it with the last line in the  
previous function.

"(b)": this is what DECL_ARTIFICIAL is for, but it has no effect if  
there's no debug information.

"If I say "step" off the end of a C++ "main", I don't want to end up  
in one of these functions": yes, that's what DECL_ARTIFICIAL is for.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2462 bytes --]

Re: Don't set DECL_IGNORED_P on a cdtor function

[Mark Mitchell]

Geoffrey Keating wrote:

>>>> The key issues are that (a) we have no source for these
>>>> compiler-generated functions, and (b) they don't correspond to anything
>>>> in the source program.  If I say "step" off the end of a C++ "main", I
>>>> don't want to end up in one of these functions; I want to end up in the
>>>> first C++ destructor being called by them.  That's the C++ view of the
>>>> world; these cdtor functions are just implementation details.

> The entire paragraph under 'Eric Botcazou wrote' lists reasons why my
> patch is correct.
> 
> "(a)": well, if you don't have source, tell the debugger that, rather
> than having the debugger associate it with the last line in the previous
> function.

In practice, what happens with your patch?  What file/line information
(if any) is associated with the function?

(I'd rather hope that the debugger was clever enough to associate
regions with no debug information with the nearby ELF (or equivalent)
object file symbols, rather than glomming it onto the previous function,
but that's a separate issue.)

> "(b)": this is what DECL_ARTIFICIAL is for, but it has no effect if
> there's no debug information.

I do see a good argument for generating debug info for these functions:
language-level constructors and destructors may be inlined into them.
So, I guess we'd ideally like to have most of the function be invisible
to the debugger, but we'd like the inlined functions to show up.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Don't set DECL_IGNORED_P on a cdtor function

[Eric Botcazou]

> In practice, what happens with your patch?  What file/line information
> (if any) is associated with the function?

The flag doesn't change anything for file/line information, that's the very 
reason why I wanted to make it stronger.

-- 
Eric Botcazou