[Bug debug/83935] DWARF for a variant part is incorrect

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83935

--- Comment #8 from Pierre-Marie de Rodat  ---
Understood, thank you for the notice! As we have to tweak the spec one way or
another for Ada, I suggest indeed we keep the way things are implemented in GCC
today, waiting for the DWARF committee to state on this. This will probably
take a while, so I’m not sure what to do with this PR. ;-)

Can you please tell me when you managed to update GDB to work with variant? It
could be interesting to see how it deals with GCC’s, and if it does not, how
much work will be needed. Thank you in advance!

[Bug lto/84213] [8 Regression] 521.wrf_r from SPEC 2017 fails to build (link) with LTO

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84213

--- Comment #18 from Pierre-Marie de Rodat  ---
Excellent, thank you for the feedback!

[Bug lto/84213] [8 Regression] 521.wrf_r from SPEC 2017 fails to build (link) with LTO

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84213

Pierre-Marie de Rodat  changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
 Resolution|--- |FIXED
   Assignee|rguenth at gcc dot gnu.org |derodat at adacore dot 
com

--- Comment #16 from Pierre-Marie de Rodat  ---
Martin, the issue should now be fixed on trunk. Thank you for reporting it! Can
you confirm everything is fine on your side?

[Bug lto/84213] [8 Regression] 521.wrf_r from SPEC 2017 fails to build (link) with LTO

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84213

--- Comment #14 from Pierre-Marie de Rodat  ---
(In reply to Pierre-Marie de Rodat from comment #13)
> IIRC, DECL_VALUE_EXPR is something like MEM_REF (-1). Yes, I’ll give it a
> try…

I submitted a candidate patch:
https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00513.html

[Bug lto/84213] [8 Regression] 521.wrf_r from SPEC 2017 fails to build (link) with LTO

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84213

--- Comment #13 from Pierre-Marie de Rodat  ---
(In reply to rguent...@suse.de from comment #11)
> This means you should be able to arrange for this
> to be emitted by tree_add_const_value_attribute_for_decl as well, no?

I don’t know… I have to dive into this again. :-)

> See above - I think we should emit this special value from
> tree_add_const_value_attribute_for_decl instead, reverting the original
> patch.
> 
> As I can't seem to get a gnat1 debugged right now can you try to
> see what would be necessary to do that?  How's the DECL_VALUE_EXPR
> looking like?

IIRC, DECL_VALUE_EXPR is something like MEM_REF (-1). Yes, I’ll give it a try…

[Bug lto/84213] [8 Regression] 521.wrf_r from SPEC 2017 fails to build (link) with LTO

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84213

--- Comment #10 from Pierre-Marie de Rodat  ---
(In reply to Richard Biener from comment #8)
> Yeah, setting DECL_PRESERVE_P would make it possible to add the
> location late since we'd not remove the decl.  Currently it's simply
> gone and the early dwarf2out_"late"_global_decl is the last thing
> we see from it...
Ok, understood.

> Currently struggling in getting a debuggable gnat1 built .. it looks
> like ./configure --enable-languages=c++,ada --disable-bootstrap
> no longer works with a host compiler that doesn't understand some
> C++ specs that appear to be used (GCC 4.8).
I’m using GCC 7.2.1. It’s odd that you get this issue only with gnat1, though…

(In reply to Richard Biener from comment #9)
> Another possibility would be to add the info symbolically by refering to the
> early DIE of the other entity.  Not sure if that works for the Ada cases? 
> We have
> DW_OP_GNU_variable_value but in this case we want the DIEs address.
In the original ADa use case, there is no other entity: the variable for which
we want to emit a location is around only for its name, so instead of
allocating a memory slot for it, we produce a “dummy” location (DW_OP_const1s:
-1). So I don’t know: would what you have in mind work? (just keeping producing
a location is enough for Ada, I think)

> Btw, if you compile any of your testcases for the issue with -g -flto
> can you verify if the generated object files contain relocations in the
> early LTO debug sections?  Those should not refer to real symbols...
> else the problem will manifest with Ada as well.
I’m confused: is this query for myself or for one Martin? ;-)

> So I think we do have to revert the revision in question (or put in the
> above hack).
Either way, in the end we need a solution that fixes the SPEC build error and
that does not make Ada debugging regress. :-)

I’m still not familiar with LTO and how early/late passes are supposed to work,
so I lack judgment about what is a hack and what isn’t. Do you think we can
find a clean way to delay location production from the “early late” pass to the
“truly late” one?

[Bug lto/84213] [8 Regression] 521.wrf_r from SPEC 2017 fails to build (link) with LTO

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84213

Pierre-Marie de Rodat  changed:

   What|Removed |Added

 CC||derodat at adacore dot com

--- Comment #7 from Pierre-Marie de Rodat  ---
Hello,

(In reply to Richard Biener from comment #6)
> I'm quite sure the rev. is responsible for similar Ada -flto -g failures. 
> We simply may _not_ add location attributes early.

I did not have a deep look at this, so I’m asking: is it possible to add it
late, then?

> So I think we need to revert this patch and instead make sure those
> Ada variables do not get removed?  Like by marking them with
> DECL_PRESERVE_P?
> 
> Trying that (figuring where those decls are created).  Pierre-Marie,
> maybe you can help here as well.

IIRC, what my r248792 is about is not to preserve the DW_TAG_varible itself (it
was already present in the DWARF output before), but to make sure we have a
location for it: if we have a DW_TAG_variable entry without a DW_AT_location
attribute, GDB will consider that the variable does not exist and thus we’ll
loose debuggability in Ada.

[Bug debug/83935] DWARF for a variant part is incorrect

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83935

--- Comment #6 from Pierre-Marie de Rodat  ---
Just got a notification that it got assigned issue #180123.1:
http://dwarfstd.org/ShowIssue.php?issue=180123.1

[Bug debug/83935] DWARF for a variant part is incorrect

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83935

--- Comment #5 from Pierre-Marie de Rodat  ---
I just submitted an Issue/Comment on dwarfstd.org, but unfortunately it is not
yet publicly visible (http://dwarfstd.org/Issues.php). Waiting for feedback
from there…

[Bug debug/83935] DWARF for a variant part is incorrect

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83935

--- Comment #4 from Pierre-Marie de Rodat  ---
(In reply to Tom Tromey from comment #3)
> TBH this did not make sense to me, either, which is partly why I originally
> wrote my patch the "more natural" way -- then this got caught in review,
> see https://reviews.llvm.org/D42082
Thanks for the pointer. So first, thanks for your efforts in adding support for
these tags in GDB!

During the review, Paul said:
> Adding a TAG_variant_part that had no children could also work, although it
> seems a little odd and might trip up an unwary non-Rust-aware debugger.
Well, for now, we do generate child-less variant parts for Ada types such as:

type Integer_Option (B : Boolean) is record
   case B is
  when False => null;
  when True  => Value : Integer;
end record;

So I hope non-Rust-aware debuggers can cope with this. ;-)

> It's worth a shot, though I think it was tried before, see
> http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2006-August/
> 001710.html
Ah, I see. Looks like I have the same reasoning that Ron and Todd with respect
to variant parts nesting had 12 years ago! :-D Everybody seems to agree that
having a top-level member to serve as a variant part’s discriminant is a good
thing to have, and it’s even suggested to post a proposal to reword the
troublesome paragraph.

So I suggest we indeed post a submission on dwarf-discuss@ and leave GCC’s
DWARF back-end as it is today. What do you think? It’s a bit late for me, so
I’ll give it a try tomorrow.

[Bug debug/83935] DWARF for a variant part is incorrect

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83935

--- Comment #2 from Pierre-Marie de Rodat  ---
Thinking more about it, the rule that the discriminant entry must be a child of
the variant part entry sounds suspicious to me.

In the case of two variant parts, which are nested and depend on the same
discriminant, where should the discriminant entry go? If it’s in the outer one,
then it’s not a child of the nested one, violating the rule, and conversely. We
could duplicate the dicriminant entry, but that does not look friendly DWARF
consumer at all: two member entries would have the same name. And that sounds
like a waste of space. Here’s the Ada example:

   --  $ gcc -c -g -fgnat-encodings=minimal pck.ads).
   package Pck is

  type Rec (I : Integer) is record
 case I is
when Positive =>
   C : Character;
   case I is
  when 0 =>
 null;
  when others =>
 N : Natural;
   end case;
when others =>
   S : String (1 .. 10);
 end case;
  end record;

  R : Rec (1);

   end Pck;

And now, even though it’s illegal in Ada (it could be legal in another
language): what about two variant parts that have the same discriminant and
that are not nested?

I guess I should report these questions to the DWARF discussion mailing list.
What do you think, Tom?

[Bug debug/83935] DWARF for a variant part is incorrect

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83935

Pierre-Marie de Rodat  changed:

   What|Removed |Added

 Status|UNCONFIRMED |ASSIGNED
   Last reconfirmed||2018-01-22
 CC||derodat at adacore dot com
  Component|ada |debug
   Assignee|unassigned at gcc dot gnu.org  |derodat at adacore dot 
com
 Ever confirmed|0   |1

--- Comment #1 from Pierre-Marie de Rodat  ---
Hello Tom,

I agree with your interpretation of the standard. I did not notice that part
when I implemented DW_TAG_variant_part generation several years ago. I’ll try
to fix that. Note however that since GCC is in stage 4, it’s likely that the
fix will not be accepted in trunk until we switch back to stage 5 (in April, I
guess).

In any case, thank you for reporting this!

[Bug debug/83765] LTO bootstrap with Ada fails

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83765

--- Comment #6 from Pierre-Marie de Rodat  ---
(In reply to Eric Botcazou from comment #4)
> We have been crossing fingers about that for some time. ;-)  CCing the
> expert.

Calling me “expert” is a bit far fetched: I never worked with LTO, and I
already have trouble understand what’s going on in dwarf2out after
optimizations. ;-) I’m relieved that Richard’s patch seems to have fixed the
issue!

[Bug driver/83016] gnat1: warning: command line option ‘-nostdinc++’ is valid for C++/ObjC++ but not for Ada

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83016

--- Comment #1 from Pierre-Marie de Rodat  ---
I can’t reproduce with “--enable-languages=c,c++,ada --disable-multilib
--disable-libsanitizer” on trunk r254797. Do you use specific options?

[Bug debug/82155] [7 Regression] ICE in dwarf2out_abstract_function, at dwarf2out.c:21655

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82155

Pierre-Marie de Rodat  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |FIXED

--- Comment #9 from Pierre-Marie de Rodat  ---
Fix is on trunk and I just merged it in the gcc-7 branch. I’ve left the target
milestone to 7.3 even though 7.4 is available since according to
https://gcc.gnu.org/gcc-7/, the last release was 7.2. Hoping I got it right!

[Bug ada/82844] [8 Regression] Many ada tests time out on x32

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82844

--- Comment #9 from Pierre-Marie de Rodat  ---
I’ve setup an Ubuntu VM, installed libx32 runtime libraries and built with
“--with-multilib-list=m32,m64,mx32” + ran the testsuite (“make -C gcc -j8 -k
check-ada”), without reproducing the issues you describe. Is there something
obvious missing?

[I’ve since resumed porting GNAT patches, so hopefully the issue will disappear
on your side]

[Bug ada/82844] [8 Regression] Many ada tests time out on x32

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82844

--- Comment #8 from Pierre-Marie de Rodat  ---
(In reply to H.J. Lu from comment #7)
> That is r253366. X32 failed to build between r253136 and r253365.

Ok, thanks! We still have patches in the pipeline in this area (s-taprop*
files), so I’ll try to port them. Hopefully this will fix the issue.

[Bug ada/82384] [8 Regression] s-taprop.adb failed to compile for x32

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82384

--- Comment #13 from Pierre-Marie de Rodat  ---
Yes, and thank you Eric for checking the fix in. :-)

[Bug ada/82384] [8 Regression] s-taprop.adb failed to compile for x32

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82384

--- Comment #7 from Pierre-Marie de Rodat  ---
I think your second test will also fail for the same reason. I suggest you also
add the following patchlet:

diff --git a/gcc/ada/libgnarl/s-osinte__x32.adb
b/gcc/ada/libgnarl/s-osinte__x32.adb
index a2874be3d69..477abcc4176 100644
--- a/gcc/ada/libgnarl/s-osinte__x32.adb
+++ b/gcc/ada/libgnarl/s-osinte__x32.adb
@@ -89,8 +89,6 @@ package body System.OS_Interface is
function To_Timespec (D : Duration) return timespec is
   S : time_t;
   F : Duration;
-
-  use type System.Linux.time_t;
begin
   S := time_t (Long_Long_Integer (D));
   F := D - Duration (S);

Unfortunately my Linux distrib does not package a runtime package for x32, so
testing will require me to install a VM and so on. I hope you’ll understand
that I’ll do that only if things get too complex on your side. Thanks again for
your help!

[Bug ada/82384] [8 Regression] s-taprop.adb failed to compile for x32

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82384

Pierre-Marie de Rodat  changed:

   What|Removed |Added

 CC||derodat at adacore dot com

--- Comment #1 from Pierre-Marie de Rodat  ---
Created attachment 42273
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=42273=edit
Tentative untested fix

Hello,

Thank you for reporting this. Could you please check that the attached untested
patch fixes the issue you are reporting? (or tell me how to reproduce on my
x86_64-linux box :-)) Thank you in advance!

[Bug debug/82155] [7/8 Regression] ICE in dwarf2out_abstract_function, at dwarf2out.c:21655

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82155

--- Comment #4 from Pierre-Marie de Rodat  ---
I have a candidate fix, which I submitted on gcc-patches@:

[Bug debug/82155] [7/8 Regression] ICE in dwarf2out_abstract_function, at dwarf2out.c:21655

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82155

Pierre-Marie de Rodat  changed:

   What|Removed |Added

 CC||derodat at adacore dot com

--- Comment #3 from Pierre-Marie de Rodat  ---
Thank you for reporting this. I’m having a look.

[Bug ada/79542] [7 regression] ICE in add_gnat_descriptive_type_attribute

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79542

Pierre-Marie de Rodat  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
  Known to work||8.0
 Resolution|--- |FIXED
Summary|[7/8 regression] ICE in |[7 regression] ICE in
   |add_gnat_descriptive_type_a |add_gnat_descriptive_type_a
   |ttribute|ttribute
  Known to fail||7.2.0

--- Comment #12 from Pierre-Marie de Rodat  ---
I just merged the fix on the gcc-7 branch and tried to restore the change
Richard did to this before the bug database backup restore.

[Bug ada/79542] [7/8 regression] ICE in add_gnat_descriptive_type_attribute

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79542

--- Comment #7 from Pierre-Marie de Rodat  ---
(In reply to John Marino from comment #6)
> It looks like the release of 7.2 is upcoming.  It would be really great if
> this ICE/Regression is addressed for that release.  Is there any way to
> raise the visibility on this issue to avoid missing a fix for 7.2?

I would be all for it. For the record, I sent a PING for my patch on
gcc-patches@ on Monday:
.

[Bug ada/35880] GNAT does not generate debugging information on imported entities

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=35880

Pierre-Marie de Rodat  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 CC||derodat at adacore dot com
 Resolution|--- |FIXED
   Assignee|unassigned at gcc dot gnu.org  |derodat at adacore dot 
com
   Target Milestone|--- |8.0

--- Comment #11 from Pierre-Marie de Rodat  ---
For your information, I just committed a fix for this: see r249449. So now, GCC
generates DWARF for imported entities.

[Bug testsuite/78318] FAIL: g++.dg/pr78112.C scan-assembler-times DW_AT_object_pointer 37

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78318

--- Comment #3 from Pierre-Marie de Rodat  ---
(In reply to Ramana Radhakrishnan from comment #1)
> Is this still an issue ? Surely isn't the fix just changing
> scan-assembler-times to 38 ?

I could not just change the count, as I got different counts from Thomas’. So
what I did was to create a smaller testcase (see PR78112) that would hopefully
get consistent results across platforms. AFAIK, nobody complained since then,
so I think we can close PR78318.

[Bug testsuite/78318] FAIL: g++.dg/pr78112.C scan-assembler-times DW_AT_object_pointer 37

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78318

--- Comment #4 from Pierre-Marie de Rodat  ---
(In reply to Thomas Preud'homme from comment #2)
> I assume the result is 37 for some platform. Anyway, I apologize since the
> scan has been removed from the testcase so this is now fixed as of r243432.

Ok, thank you Thomas!

[Bug ada/79542] [7 regression] GNAT bug box

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79542

Pierre-Marie de Rodat  changed:

   What|Removed |Added

 CC||derodat at adacore dot com

--- Comment #3 from Pierre-Marie de Rodat  ---
Thank you for CCing me, Jakub.

This is just to say that I will not be able to work on this until March. If
this can wait until then, I’ll be happy to investigate and try to fix the
crash.

[Bug debug/78839] [6/7 Regression] DWARF output different between GCC 5 and 6

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78839

Pierre-Marie de Rodat  changed:

   What|Removed |Added

 CC||derodat at adacore dot com

--- Comment #9 from Pierre-Marie de Rodat  ---
(In reply to Jakub Jelinek from comment #7)
> The behavior changed with r231762, I haven't looked (yet) into details if it
> is valid or not and why that changed.
I haven’t had a look yet neither… bit addressing is hard. ;-) All I can do for
now is to give some context for r231762: this commit tried to handle fields
with dynamic sizes/positions but only with a focus on the ones at least
byte-aligned.

My playground for this was discriminated types in Ada, and I’m not sure these
can be bit-packed (i.e. using sub-byte addressing). So if anything changed for
bitfields, that was not intentional and can likely be fixed as long as it does
not break things for the Ada types I tested. By the way, if one wants to see
what I used for testing, here it is:
https://github.com/pmderodat/dwarf-ada-testsuite/.

[Bug debug/78112] [7 regression] invalid DWARF generated by the compiler: DIE has multiple AT_inline attributes

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78112

Pierre-Marie de Rodat  changed:

   What|Removed |Added

 CC||thopre01 at gcc dot gnu.org

--- Comment #17 from Pierre-Marie de Rodat  ---
For the record, Thomas also created a PR for this matter on arm-none-eabi:
PR78318. I’ll close it.

Thank you all for your reports. I tried to reproduce with partial
cross-compilers on my x86_64-linux box but got still different results (for
instance, I have 40 times DW_AT_object_pointer on arm-none-eabi). I also tried
with an even more reduced reproducer (the one I quoted above) but again, I get
different counts with the various compilers.

The fact that there are duplicate attributes (as I said above: even with my fix
and before my original patch) does not help. At this point I’m considering just
removing the testcase… and hope that eventually I’ll be able to add a true one
once duplicates are all gone. :-/

Thoughts?

[Bug debug/78112] [7 regression] invalid DWARF generated by the compiler: DIE has multiple AT_inline attributes

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78112

Pierre-Marie de Rodat  changed:

   What|Removed |Added

 Status|UNCONFIRMED |RESOLVED
 CC||derodat at adacore dot com
 Resolution|--- |FIXED
   Assignee|unassigned at gcc dot gnu.org  |derodat at adacore dot 
com

--- Comment #13 from Pierre-Marie de Rodat  ---
I just pushed the suggested fix on trunk, so the regression should be gone,
now.

[Bug debug/78112] [7 regression] invalid DWARF generated by the compiler: DIE has multiple AT_inline attributes

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78112

--- Comment #10 from Pierre-Marie de Rodat  ---
Created attachment 39932
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=39932=edit
pyelftools-based Python3 script to check duplicate attributes in ELF/DWARF info

[Bug debug/78112] [7 regression] invalid DWARF generated by the compiler: DIE has multiple AT_inline attributes

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78112

--- Comment #9 from Pierre-Marie de Rodat  ---
Back to business. ;-)

Thank you very much, Jakub! During investigation, I managed to reduce this even
further:

void run (int *int_p, void(*func)(int *)) { func (int_p); }
namespace foo {
   struct Foo {
  int a;
  Foo() { run (, [](int *int_p) { *int_p = 0; }); }
   };
}
int main (void) { foo::Foo f; }

With today’s trunk (gcc/cc1plus -g -dA foo.C) and this reproducer, I see three
DW_TAG_subprogram having two DW_AT_object_pointer each.

Disclaimer: to be perfectly honnest, I have a very partial understanding of how
dwarf2out.c:gen_subprogram_die is supposed to work. It has a good share of
explaining comments, but it is still hard to grasp as it does quite complex
things. That being said…

My analysis so far is that gen_subprogram_die is supposed to be called a very
specific number of times for each function and my change from 2016-10-12 made
it run more times, so it unintentionally added some attributes more than just
once. The following patch seems to restore call count to what we had
previously:

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 78a2979..a85ab3b 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -23909,11 +23909,11 @@ dwarf2out_early_global_decl (tree decl)
  if (!DECL_STRUCT_FUNCTION (decl))
goto early_decl_exit;

- /* For nested functions, emit DIEs for the parents first so that all
-nested DIEs are generated at the proper scope in the first
-shot.  */
+ /* For nested functions, make sure we have DIEs for the parents first
+so that all nested DIEs are generated at the proper scope in the
+first shot.  */
  tree context = decl_function_context (decl);
- if (context != NULL)
+ if (context != NULL && lookup_decl_die (context) == NULL)
{
  current_function_decl = context;
  dwarf2out_decl (context);

I’m currently bootstrapping+regtesting it on x86_64-linux. I tried to come up
with a regression testcase for it, but I can’t find a human-friendly regexp for
scan-assembler-times to check that we don’t have multiple attributes per tag.
So for local testing, I wrote a Python3 script to use the pyelftools library.
I’ll file it right after sending this message if others need to use it.

Something bugs me, though: with Jakub’s reproducer, I get multiple
DW_AT_object_pointer attributes even at r241022 (i.e. right before my
2016-10-12 patch):

$ gcc/cc1plus -g -dA pr78112.ii && gcc -c pr78112.s && ./check.py pr78112.o
In (0x3c4) DW_TAG_subprogram: DW_AT_object_pointer appears 2 times
In (0x46a) DW_TAG_subprogram: DW_AT_object_pointer appears 2 times
In (0x4fe) DW_TAG_subprogram: DW_AT_object_pointer appears 2 times
In (0x55a) DW_TAG_subprogram: DW_AT_object_pointer appears 2 times
In (0x5cf) DW_TAG_subprogram: DW_AT_object_pointer appears 2 times
In (0x668) DW_TAG_subprogram: DW_AT_object_pointer appears 2 times

So I’m surprised Rainer hasn’t spotted this kind of error before.

Thoughts?

[Bug debug/78112] [7 regression] invalid DWARF generated by the compiler: DIE has multiple AT_inline attributes

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78112

Pierre-Marie de Rodat  changed:

   What|Removed |Added

 CC||derodat at adacore dot com

--- Comment #7 from Pierre-Marie de Rodat  ---
(In reply to Rainer Orth from comment #5)
> I'm attaching a tarball with preprocessed input (identical except for path
> names) and the assembler output.  The one at r241022 is fine, while dsymutil
> complains about the second (r241023) and indeed in the assembler output at
> line 57708 there's a duplicate DW_AT_inline.

Thank you for reporting! I’m on vacation until the end of the week; I’ll try to
reproduce and understand what is going on next week.

[Bug rtl-optimization/66790] Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

--- Comment #43 from Pierre-Marie de Rodat  ---
(In reply to Bernd Schmidt from comment #42)
> I don't think that would actually help. Even if something is an actual
> incoming argument register, it may still be uninitialized by the caller.
Sure, but my understanding is that what matters in DF is what happens inside
the function, so we can assume arguments are always initialized.

Actually this makes sense in the context of REE as well: we have one
(artificial) def for arguments so since this def has no corresponding
instruction, REE will not be able to remove z-extensions reaching this def.

That being said, I failed to get the “is it really used for incoming arguments”
hard-reg predicate, so falling back to your proposal: I will followup on
gcc-patches@.

Thanks! :-)

[Bug rtl-optimization/66790] Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

--- Comment #41 from Pierre-Marie de Rodat  ---
Thank you again for spotting this! Yes, these artificial defs break the
consevativeness for the MIR analysis. I guess your proposal would work:
considering as uninintialized registers than aren’t is conservative
although it will prevent more optimizations that one could expect in
REE. I don’t know if it’s a problem in practice but your example shows
that we have a lot of registers in this situation.

At this point, though, I wonder why artificial defs do not describe what
actually happens for this function. Shouldn’t we instead fix DF to emit
artificial defs only for actual input registers? I’ll investigate this.

… and then I will followup on gcc-patches as you suggested. Once more,
thank you all for your help in this! :-)

[Bug rtl-optimization/66790] Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

--- Comment #38 from Pierre-Marie de Rodat  ---
(In reply to Paolo Bonzini from comment #35)
> Is this comment obsolete?  The IN bitmap is all set at first.
Yes, indeed: I removed the part you quoted.

(In reply to Paolo Bonzini from comment #35)
> Otherwise looks good.
Thank you! Does this include the DF_LIVE fix?


(In reply to Bernd Schmidt from comment #36)
> This looks better. I still don't quite understand why you're treating
> MUST_CLOBBER and MAY_CLOBBER defs differently in simulate. It looks
> like a MUST_CLOBBER produces a bit in gen which I think is not what is
> wanted.
> 
> Anything wrong with writing this simply as follows?
(In reply to Paolo Bonzini from comment #37)
> Bernd is right that you have a missing 'else'.
Pastos strike back! I updated df_mir_simulate_one_insn according to your
suggestion: thank you.

Besides, Eric told me live that instead of artificially clearing the
entry basic block’s IN set, we could use the confluence_0 for that. I
did this and took this opportunity to minimize code duplication: the
df_mir_init function now just calls df_mir_reset.

> And, not as a requirement for your patch, but as a point for
> discussion - do we want a special all_ones_bitmap that doesn't take up
> memory for purposes like this? It would add two additional tests to
> each bitmap_{and,ior} operation.

Good point, although what would the following yield, then?

bitmap_clear (res);
bitmap_set_bit (res, 1);
bitmap_and_into (res, all_ones_bitmap);

I mean: we would need to represent bitmaps will all bits set but one
(and by extension: all but several).

Anyway, here's the updated patch, bootstrapped and regtested again on
x86_64-linux (plus a new testcase in gnat.dg). By the way, there's still
the MIR name, as we did not reach a consensus about a better name.

[Bug rtl-optimization/66790] Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

Pierre-Marie de Rodat  changed:

   What|Removed |Added

  Attachment #36377|0   |1
is obsolete||

--- Comment #39 from Pierre-Marie de Rodat  ---
Created attachment 36384
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=36384=edit
Updated candidate patch

[Bug rtl-optimization/66790] Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

--- Comment #32 from Pierre-Marie de Rodat  ---
(In reply to Bernd Schmidt from comment #28)
> It is sufficient for OUT(3) to be all-zeros. And I don't think the
> LAST_CHANGE_AGE mechanism does anything to prevent it. Please try it
> out. I think you have to initialize your bitmaps correctly rather than
> rely on "visited".
Thank you very much for the reproducer: I confirm that the
LAST_CHANGE_AGE mechanism does not have the effect I thought. I just
updated my patch locally to initialize bitmaps to 1 for all registers
and thus remove the visited field: this fixes the issue you found.

> > (In reply to Bernd Schmidt from comment #14)
> > > I do have to say that I am still uncomfortable with changing RRE to
> 
> I did not write this.
Indeed: Kenneth Zadeck said this in comment #20. I made a pasto, sorry
for that!



(In reply to Paolo Bonzini from comment #29)
> BTW, are you sure that
> 
> +  if (DF_REF_FLAGS_IS_SET (def,
> +  DF_REF_PARTIAL | DF_REF_CONDITIONAL))
> +   /* All partial or conditional def
> +  seen are included in the gen set. */
> +   bitmap_set_bit (gen, regno);
> 
> ?  I would have thought they don't belong in any set, and on the
> contrary I would have thought that may-clobber definitions count as
> kills.
For partial and conditional defs in the context of MIR:

  * if the register was initialized, then it is still initialized
afterwards, whatever happens;

  * if the register was uninitialized, then in the case of partial def
there will still be bits uninitialized and in the case of
conditional def it is possible that the instruction leaves the
register uninitialized: in both case there is a possibility to leave
part of the register uninitialized.

So I would agree with: they don't belong in any set.

While thinking about this, I also realized with REE in mind that since
we need a conservative computation for MIR, we must set KILL/clear GEN
for register refs with DF_REF_MAY_CLOBBER: it may leave the register
uninitialized.

(In reply to Paolo Bonzini from comment #31)
> Ah, I see now.  I think you're right that the DF_REF_MUST_CLOBBER case
> should also clear GEN in df_live_bb_local_compute.
> 
> However, regarding the "BTW" I am fairly sure now that
> df_live_bb_local_compute and the corresponding function for MIR should
> handle may-clobber and may-sets differently.  If you think of
> may-clobber and may-set as a diamond-shaped CFG:
> 
> […]
> 
> Then at the join point you have an "OR" for LIVE (so the clobber's
> KILL disappears and the set's GEN remains), and an "AND" for MIR.  For
> MIR the clobber's KILL remains and the set's GEN disappears.
Agreed, thank you! I have updated both MIR and LIVE in the light of
this. (bootstrapped and regtested on x86_64-linux)

[Bug rtl-optimization/66790] Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

Pierre-Marie de Rodat  changed:

   What|Removed |Added

  Attachment #36098|0   |1
is obsolete||

--- Comment #33 from Pierre-Marie de Rodat  ---
Created attachment 36377
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=36377=edit
Updated candidate patch

[Bug rtl-optimization/66790] Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

--- Comment #34 from Pierre-Marie de Rodat  ---
Created attachment 36378
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=36378=edit
Fix for DF_LIVE local BB information

[Bug rtl-optimization/66790] Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

--- Comment #27 from Pierre-Marie de Rodat  ---
Firstly, thanks everyone for your help! I'll try to address points that
still are unresolved.



(In reply to Bernd Schmidt from comment #14)
> As you say, the df-live problem claims to compute a must-initialized
> problem, so it would be very odd to add another problem with the same name.
(In reply to Eric Botcazou from comment #21)
> (In reply to Paolo Bonzini from comment #19)
>> There's no code in GCC for must-initialized.  Pierre's patch gets it right
>> (except that I'm not sure about the comment at line 2033).  The MIR name can
>> indeed add to the confusion about may/must.  Perhaps valid registers (VR)?
> 
> Thanks for the comment.  Yes, I agree that the MIR name ought to be improved.
To me, "valid" register has the same meaning as "initialized" and thus
does not carry the information about whether we can assume it is either
always or sometimes valid/initialized. So what about "always initialized
registers"? The acronym would be AIR.



(In reply to Bernd Schmidt from comment #14)
> I experimented with changing df_live_confluence_n to use and instead of ior,
> followed by actually taking the confluence_n from your mir problem. However,
> this does not work: ior enlarges the set while and shrinks it, so the
> initialization of the bitmaps would have to be different.
Yes: they both compute different information as discussed later in this
thread.



(In reply to Bernd Schmidt from comment #14)
> It looks like your "visited" bit tries to avoid that, but I don't
> think this works.
(In reply to Paolo Bonzini from comment #19)
> There's no code in GCC for must-initialized.  Pierre's patch gets it right
> (except that I'm not sure about the comment at line 2033).
The "visited" bit I introduced is used to fake an "all bits set"
initialization. The confluence_n method is meant to do:

IN(dest) = OUT(src_0) AND OUT(src_1) AND ...

... by small increments of:

IN(dest) := IN(dest) AND OUT(src_i)

When it is executed for the first time, IN(dest) is all-zeros, so we do
a copy because "a AND all_ones = a". Then we can do regular AND later
on. In order to get rid of this "visited" bit, I guess we could instead
pretend that IN/OUT sets are set of never-or-may-initialized registers,
but this complicates other parts of the code, so...

Paolo, does this clarify the comment at line 2033? (I will update it if
you think this is enough :-))



(In reply to Bernd Schmidt from comment #14)
> One testcase I looked at had this structure:
> 
> entry -> 2 -> 3 -> 3
> 
> i.e. a loop in  block 3. The confluence_n function takes the out bitmap from
> block 3 (which is all zeros) and ands it with the in bitmap from the same
> block, which has the incoming regs from block 2 at that point, but obviously
> the bitmap turns into all zeros at this point (block 3 was already visited
> due to the 2->3 edge). I don't see how your patch doesn't suffer from the
> same problem.
I don't understand what the problem is: the confluence_n function is
used during forward propagation. So in your example:

 a. the confluence_n function will be called for the 3 -> edge only
after...

 b. the transfer function is invoked for the basic block 3 (then OUT(3)
is initialized and potentially not all-zeros); this will be invoked
only after...

 c. the confluence_n function is invoked for the 2 -> 3 edge
(then IN(3) is initialized and potentially not all-zeros).

If I understand correctly, the BB_LAST_CHANGE_AGE mechanism in df-core.h
makes sure computations comply with this dependency (c. before b., b.
before a.).  So at the point the confluence_n function for the 3 -> 3
edges is called for the first time, both IN(3) and OUT(3) can be not
all-zeros.



(In reply to Bernd Schmidt from comment #14)
> I do have to say that I am still uncomfortable with changing RRE to
> use a MUST problem rather than a MAY problem.   I see this as dumbing
> down the compiler to provide the semantics of uninitialized variables
> and it is a path that we have generally avoided in GCC. I do not have
> a better solution, but there is a feeling that something is being
> missed here.
Indeed, while trying to fix the bug, I noticed that no RTL pass needs
this "is initialized on all paths" information.

I'm not sure what "having no semantics for uninitialized variables"
really implies: does this mean that we have an undefined behavior as
soon as code uses the value of an uninitialized variable? If not, I
think what makes REE special regarding this is that zero-extension is
used to pack independent values into single storage units (registers).
Because of this, considering that the whole storage unit is
uninitialized because a single embedded value is uninitialized makes us
generate wrong code.



Now on a different topic, I forgot to mention with my first candidate
patch: LIVE and MIR are supposed to work on the same GEN/KILL sets.
These respectively represent the 

[Bug rtl-optimization/66790] Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

--- Comment #13 from Pierre-Marie de Rodat  ---
(In reply to Eric Botcazou from comment #12)
> Thanks.  I misremembered, the testcase has a single variable with two
> fields, one uninitialized and one initialized, instead of two variables, but
> it's exactly the same reasoning (and it would be trivial to add the two
> variables anyway).

Thank you for your involvement into this! :-)

[Bug rtl-optimization/66790] Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

--- Comment #11 from Pierre-Marie de Rodat  ---
Created attachment 36320
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=36320=edit
Reproducer with an uninitialized variable (no OUT parameter)

[Bug rtl-optimization/66790] Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

--- Comment #8 from Pierre-Marie de Rodat derodat at adacore dot com ---
(In reply to Richard Biener from comment #7)
 It would be certainly good to see why we have this UNDEF in the first place.

Sure: here is a C translation of what happens in my Ada reproducer (only wrt.
UNDEF: this is not a reproducer for the original bug).

#include stdint.h

struct option_t {
  uint16_t is_present;
  uint16_t value;
};

struct option_t
foo (uint16_t a, uint16_t b)
{
  struct option_t result;

  result.is_present = b != 0;
  if (result.is_present)
result.value = a / b;
  return result;
}

On x86, foo's return value is the 32-bit value in $eax. If b == 0, its lower
half is undefined while its upper one is supposed to be 0. In other words, even
though the value field is uninitialized, the is_present one must be 0. Although
a trunk GCC produces no zero-extension at -O2 for this example, this simply
exposes the source use case for the half-initialized value business.

 But yes, I have to agree that zext UNDEF  16 should be zero.  Unlike
 sext UNDEF which is always undefined as the sign-bit is undefined.

Good point! The patch I submitted previously pessimized the sign-extension
case. The one I'm going to upload fixes this: bootstrapped and regtested
successfuly on x86_64-linux.

[Bug rtl-optimization/66790] Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

--- Comment #9 from Pierre-Marie de Rodat derodat at adacore dot com ---
Created attachment 36098
  -- https://gcc.gnu.org/bugzilla/attachment.cgi?id=36098action=edit
Updated candidate patch

[Bug rtl-optimization/66790] Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

--- Comment #6 from Pierre-Marie de Rodat derodat at adacore dot com ---
Thanks for your answer, Richard!

(In reply to Richard Biener from comment #5)
 So what is the issue with replacing zero-extending an uninitialized %ebp
 with a random other value?  Both are essentially undefined when reached via
 the somlabel bypass.

Well, at least on x86, even when %ebp is uninitialized, “movzwl %bp, %ebp”
makes its upper half zero (and yes, the lower half is uninitialized). Hence a
following “shr $0x10, %ebp” is supposed to leave zero in %epb, for instance. If
we have a random value instead as shr’s input, this does not work anymore, so I
consider this transformation is an error.

 Is the real issue happening in a downstream pass?  I don't think REE does
 anything wrong here.

REE is the pass where I observed the change in behavior I described in my first
message, and I did not notice anything weird beyond this. If the above is wrong
(i.e. if we cannot assume (zext UNDEF from i16 to i32)  16 == 0), then I
guess I’ll have to look for something wrong up in the pipe. ;-)

[Bug rtl-optimization/66790] Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

--- Comment #4 from Pierre-Marie de Rodat derodat at adacore dot com ---
(In reply to Pierre-Marie de Rodat from comment #0)
 Given the somelabel code path, I would rather expect DF_REF_CHAIN to hold
 a NULL reference to materialize the lack of initialization in one path. I
 found the DF_LR_IN/DF_LR_OUT macros from df.h: they provide information
 about uninitialized variables but the associated comment says they should
 not be used (This intolerance should eventually be fixed.). Besides, they
 provide a basic-block-level information whereas we are rather interested in
 instruction-level information in REE.

Having read more thoroughly dataflow code, I saw the DF_LIVE problem claims
that it provides the logical AND of the IN and OUT sets from the LR problem
and the must-initialized problem, which would be useful: it could provide a
conservative set of registers that *may* be uninitialized (i.e. all registers
that may be uninitialized are included).

So I started looking at the DF_LIVE results, but I got strange results. For
instance, with a very simple C function:

extern void do_nothing (void);

int foo (int i, int b) {
  int r;
  if (b) { do_nothing (); r = i; }
  do_nothing ();
  return r  0x;
}
/* foobar.c */

… the reference to the register holding r at the return statement is present
in the IN set for the corresponding basic block:

 $ gdb --args cc1 -O2 foobar.c
(gdb) b find_removable_extensions
(gdb) r
(gdb) p dump_function_to_file(cfun.decl, stderr, 0)
[...]
(note 13 12 14 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
(call_insn 14 13 15 4 [...])
(insn 15 14 21 4 (set (reg:SI 0 ax [orig:92 D.1771 ] [92])
(zero_extend:SI (reg:HI 3 bx [orig:87 r ] [87])))
foobar.c:13 136 {*zero_extendhisi2}
(nil))
[...]
(gdb) p df_live_top_dump(cfun.cfg.x_basic_block_info.m_vecdata[4], stderr)
;; live in 3 [bx] 7 [sp]
;; live gen 0 [ax]
;; live kill

This looks wrong to me: there is a path from the entry to this basic block that
doesn't cross a definition for the bx register, so it's not must-initialized
and thus should not be present in this set. Digging further, I noticed that in
df_live_confluence_n, we do:

IN(succ) = OUT(pred) OR IN(succ)

… which, I think, should instead be an AND with respect to the comment: we
want to keep only registers that are must-initialized, so as soon as one *may*
be uninitialized it is out of the result. But this is how it works since the
dataflow merge in 2006… and doing this change unsurprisingly makes the compiler
generate wrong code.

So assuming the LIVE pass actually does not compute must-initialized registers
(but instead what I would call maybe-initialized registers), I tried to add
another problem: MIR (Must-Initialized Registers) and to use it in the REE
pass. I'm about to submit the patch on gcc-patches@.

[Bug rtl-optimization/66790] Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

--- Comment #1 from Pierre-Marie de Rodat derodat at adacore dot com ---
Created attachment 35923
  -- https://gcc.gnu.org/bugzilla/attachment.cgi?id=35923action=edit
Part of the reproducer

[Bug rtl-optimization/66790] New: Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

Bug ID: 66790
   Summary: Invalid uninitialized register handling in REE
   Product: gcc
   Version: 6.0
Status: UNCONFIRMED
  Severity: normal
  Priority: P3
 Component: rtl-optimization
  Assignee: unassigned at gcc dot gnu.org
  Reporter: derodat at adacore dot com
  Target Milestone: ---

The reproducer I'm about to attach[1] seems to trigger a code generation issue
at least on x86_64-linux:

$ gnatmake -q p -O3 -gnatn
$ ./p

raised PROGRAM_ERROR : p.adb:9 explicit raise

The bottom line is that when Success is False in Q.Get (q.adb, around line 40)
its value is clobbered during return. If we build with -fno-ree, we can see in
the assembly code (near the return point for q__get) the following sequence:

movzwl(%rax), %epb
...
somelabel:
...
movzwl%bp, %ebp
...
salq$16, %rax
orq%rbp, %rax

However, without the -fno-ree switch the instruction:

movzwl%bp, %ebp

is merged with its definition (the first movzwl instruction) by the REE pass.
This is wrong since the definition does _not_ dominate this zero-extension: the
first movzwl instruction can be by-passed through somelabel.

I started to investigate this issue: the REE pass is currently not aware of
this by-pass code path because it is reached by no proper definition (it brings
an uninitialized %rbp register to the zero-extension). This happens in
ree.c:get_defs; in our case (insn=second movzwl, reg=BP) DF_REF_CHAIN contains
only one definition: the movzwl instruction.

Given the somelabel code path, I would rather expect DF_REF_CHAIN to hold a
NULL reference to materialize the lack of initialization in one path. I found
the DF_LR_IN/DF_LR_OUT macros from df.h: they provide information about
uninitialized variables but the associated comment says they should not be used
(This intolerance should eventually be fixed.). Besides, they provide a
basic-block-level information whereas we are rather interested in
instruction-level information in REE.

I was thinking that REE may not be the only optimization pass needing this kind
of information but I found no other one, so I'm not sure how this ought to be
handled. Can anybody confirm my intuition about the NULL reference in
DF_REF_CHAIN? I'm willing to implement it but I prefer first to be sure this is
the right approach.

Thanks in advance!


[1] It's in Ada. I tried to translate it into C but any change in register
allocation makes the issue disappear...

[Bug rtl-optimization/66790] Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

--- Comment #2 from Pierre-Marie de Rodat derodat at adacore dot com ---
Created attachment 35924
  -- https://gcc.gnu.org/bugzilla/attachment.cgi?id=35924action=edit
Part of the reproducer

[Bug rtl-optimization/66790] Invalid uninitialized register handling in REE

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66790

--- Comment #3 from Pierre-Marie de Rodat derodat at adacore dot com ---
Created attachment 35925
  -- https://gcc.gnu.org/bugzilla/attachment.cgi?id=35925action=edit
Part of the reproducer

[Bug debug/66503] New: missing DW_AT_abstract_origin for cross-unit call sites

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66503

Bug ID: 66503
   Summary: missing DW_AT_abstract_origin for cross-unit call
sites
   Product: gcc
   Version: 5.0
Status: UNCONFIRMED
  Severity: normal
  Priority: P3
 Component: debug
  Assignee: unassigned at gcc dot gnu.org
  Reporter: derodat at adacore dot com
  Target Milestone: ---

Created attachment 35754
  -- https://gcc.gnu.org/bugzilla/attachment.cgi?id=35754action=edit
C reproducer

With the recent work for PR debug/65549, we observed a regression in the
generated debugging information. Take the attached reproducer, build it and
look at the output DWARF:

 $ gcc -c -O1 -g foo.c
 $ objdump --dwarf=info foo.o
 [...]
  24e: Abbrev Number: 3 (DW_TAG_GNU_call_site)
 4f   DW_AT_low_pc  : 0x9
  257: Abbrev Number: 0

There is no DW_AT_abstract_origin attribute anymore, whereas there used to be
one.

On PowerPC with -mlongcall, for instance, call instructions are indirect and we
(at AdaCore) rely on this attribute to determine what function is actually
called (it's easier this way than interpreting machine code...). The DWARF
proposal for call sites also say debuggers should be able to use this attribute
(to build virtual call stacks in the presence of tail calls), but I could not
observe this in practice with GDB.

(Excepts from the https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00712.html
thread)

As discussed on the corresponding thread on gcc-patches@, I am about to commit
a fix on mainline and on the 5.0 branch.

[Bug debug/53927] wrong value for DW_AT_static_link

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53927

--- Comment #21 from Pierre-Marie de Rodat derodat at adacore dot com ---
(In reply to Eric Botcazou from comment #18)
 I think this is worth investigating though because it's conceptually
 much simpler than adding yet another indirection.  And we should
 concentrate on -O0 (and -Og), we don't really care about what happens
 with aggressive optimization.

Understood and agreed. Nevertheless...

 I guess the question is: can we arrange to have a constant offset
 between the frame base and the FRAME object, constant meaning valid
 for every function but possibly target-dependent?

I started to hack into cfgexpand.c and dwarf2out.c, but I realized this
is not possible in the general case. Consider the following example:

#include stdlib.h

int
nestee (void)
{
  int a __attribute__((aligned(64))) = 1234;

  void
  nested (int b)
  {
a = b;
  }

  nested (2345);
  return a;
}

int
call_nestee (int n)
{
  int *v = alloca (sizeof (int) * n);
  v[0] = nestee ();
  return v[0];
}

int
main (void)
{
  call_nestee (1);
  call_nestee (8);
  return 0;
}

With a GCC 5.0 built from fairly recent sources, I get the following CFA
information:

0090 002c 0064 FDE cie=0030
pc=004004ac..004004eb
  DW_CFA_advance_loc: 5 to 004004b1
  DW_CFA_def_cfa: r10 (r10) ofs 0
  DW_CFA_advance_loc: 9 to 004004ba
  DW_CFA_expression: r6 (rbp) (DW_OP_breg6 (rbp): 0)
  DW_CFA_advance_loc: 5 to 004004bf
  DW_CFA_def_cfa_expression (DW_OP_breg6 (rbp): -8; DW_OP_deref)
  DW_CFA_advance_loc: 38 to 004004e5

And now here is what I get under GDB:

$ gdb -n -q -ex 'b nestee' ./dyn_frame
Reading symbols from ./dyn_frame...done.
Breakpoint 1 at 0x4004c3: file dyn_frame.c, line 6.
(gdb) r
[...]

Breakpoint 1, nestee () at dyn_frame.c:6
6 int a __attribute__((aligned(64))) = 1234;
(gdb) p $pc
$1 = (void (*)()) 0x4004c3 nestee+23
(gdb) x/1xg $rbp - 8
0x7fffdf28: 0x7fffdf60
(gdb) p/x (char *) 0x7fffdf60 - (char *) a
$2 = 0xa0

... so for this frame, the CFA and the FRAME object are 0xa0 bytes from
each other. Now let's resume to see the next nestee frame:

(gdb) c
Continuing.

Breakpoint 1, nestee () at dyn_frame.c:6
6 int a __attribute__((aligned(64))) = 1234;
(gdb) p $pc
$3 = (void (*)()) 0x4004c3 nestee+23
(gdb) x/1xg $rbp - 8
0x7fffdf28: 0x7fffdf50
(gdb) p/x (char *) 0x7fffdf50 - (char *) a
$4 = 0x90

The offset between the CFA and e FRAME object is now 0x90 bytes. So
because of alignment constraints, I think we cannot assume we can have a
constant offset (even function-dependent).

This offset is dynamic and the only way to compute it is to use the
frame's context: here, nestee's saved registers, which we don't have
access to in DWARF when computing the static link attribute.

[Bug debug/53927] wrong value for DW_AT_static_link

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53927

--- Comment #22 from Pierre-Marie de Rodat derodat at adacore dot com ---
(In reply to Tom Tromey from comment #20)
 Yeah.  There wasn't much point submitting it when it wouldn't work anyhow :}
 Also see the README.archer file.  It explains some changes that are needed.
 Also I remember thinking that the dwarf locexpr baton changes needed
 some update, but I no longer recall what.
 
 I see now that I recorded my thoughts in the commit messages.
 Yay me!

Great, thank you for this!

[Bug debug/53927] wrong value for DW_AT_static_link

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53927

--- Comment #17 from Pierre-Marie de Rodat derodat at adacore dot com ---
(In reply to Tom Tromey from comment #16)
 I'm curious if you tried it on the test case in this PR.

I did not, but it looks like it now works as expected. Here are the frame base
info for nestee and the static link info for nested:

[nestee]
69   DW_AT_frame_base  : 1 byte block: 9c (DW_OP_call_frame_cfa)
[nested]
aa   DW_AT_static_link : 6 byte block: 91 60 6 23 20 6(DW_OP_fbreg:
-32; DW_OP_deref; DW_OP_plus_uconst: 32; DW_OP_deref)

... and the CFA for the two locations we will be interrested in later:

   LOC   CFA
00400496 rsp+8
00400497 rsp+16
0040049a rbp+16   # A
004004af rsp+8

   LOC   CFA
0040053b rsp+8
0040053c rsp+16
0040053f rbp+16   # B
00400564 rsp+8

Now in GDB:

$ gdb -n -q -ex 'b pr53927.c:7' -ex r --args ./pr53927
[...]
Breakpoint 1, nested (nested_arg=10) at pr53927.c:7
7   return nested_arg + 23 + self_call; /* Break here */
(gdb) bt
#0  nested (nested_arg=10) at pr53927.c:7
#1  0x0040052b in nestee (computer=0x7fffdf24, arg=10) at
pr53927.c:13
#2  0x0040051d in nestee (computer=0x40052d misc, arg=5) at
pr53927.c:11
#3  0x0040055e in main (argc=1, argv=0x7fffe058) at
pr53927.c:23

(gdb) p $pc
$1 = (void (*)()) 0x4004a4 nested+14
# This PC corresponds to the B line above: CFA = $rbp + 16
(gdb) x/1gx $rbp + 16 - 32
0x7fffdea0: 0x7fffdf20
(gdb) x/1gx 0x7fffdf20 + 32
0x7fffdf40: 0x7fffdf60

... so the static link expression gives us the following frame base address:
0x7fffdf60

(gdb) f 2
#2  0x0040051d in nestee (computer=0x40052d misc, arg=5) at
pr53927.c:11
11  arg = nestee (nested, arg + 5, 0);
# This PC corresponds to the A line above: CFA = $rbp + 16
(gdb) p/x $rbp + 16
$2 = 0x7fffdf60

So the static link expression correctly yields the frame 2's CFA.

 Yeah, growing these is to be avoided.
 My patch for this added a method to symbol_computed_ops instead.
 
 Unfortunately gitorious is acting weird so you can't see the patch
 online :-(.  But you can fetch from
 https://gitorious.org/binutils-gdb/gdb.git
 and look at the branch static-link-fix if you like.

Interesting! Having a look at this: thank you! I guess you waited for the GCC
issue to be solved before submitting them to GDB?

[Bug debug/53927] wrong value for DW_AT_static_link

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53927

--- Comment #15 from Pierre-Marie de Rodat derodat at adacore dot com ---
(In reply to Pierre-Marie de Rodat from comment #13)
 [1] This patch teaches GDB how to use DW_AT_static_link in order to find the
 frame corresponding to the lexically enclosing scope. I think I will try to
 submit it to GDB soon.

For the record, here it is:
https://www.sourceware.org/ml/gdb-patches/2015-03/msg00040.html.

[Bug debug/53927] wrong value for DW_AT_static_link

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53927

--- Comment #14 from Pierre-Marie de Rodat derodat at adacore dot com ---
Created attachment 34868
  -- https://gcc.gnu.org/bugzilla/attachment.cgi?id=34868action=edit
patch to generate DWARF-compliant DW_AT_static_link attributes

gcc/
* tree-nested.c (finalize_nesting_tree_1): Append a field to
hold the frame base address.
* dwarf2out.c (gen_subprogram_die): Generate for
DW_AT_static_link a location description that computes the value
of this field.

[Bug debug/53927] wrong value for DW_AT_static_link

[derodat at adacore dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53927

Pierre-Marie de Rodat derodat at adacore dot com changed:

   What|Removed |Added

 CC||derodat at adacore dot com

--- Comment #13 from Pierre-Marie de Rodat derodat at adacore dot com ---
(In reply to Tom Tromey from comment #8)
  Yes, but you can do something useful even with this value of
  DW_AT_static_link, albeit not exactly what DWARF means.
 
 Regardless, I think GCC should emit correct DWARF.

I gave it a try: see the attached patch.

Jason suggested to change DW_AT_frame_base in order to make it equal to the
address of the FRAME object. I was not sure:

 1) how to do it: location descriptions for all local variables would need to
be updated;
 2) whether it's safe to do this: what if optimizers move/duplicate this FRAME
object in the stack frame or do similar disturbing things? I'm not familiar
enough with optimization passes to estimate if it's likely: feedback welcome.
:-)

I thought: why not make DW_AT_static_link compute the parent frame base address
from the current static link argument? Well, when generating DW_AT_static_link
for a nested subprogram, we do not know yet the offset between the FRAME object
and the frame base address. This is because nested subprograms reach the
back-end before their parent. Besides, see point 2: are we only sure that such
a constant offset exists?

So instead, I patched tree-nested.c in order to append a field at the end of
the FRAME object to hold the frame base address (computed with the DWARF_CFA
builtin). Then, dwarf2out.c just has to emit a location description for nested
subprograms' DW_AT_static_link that fetches this using the static link
argument. It's consuming a little stack space, I'm not sure if it's a problem.
Once again: feedback welcome!

Besides, this does not work yet with Fortran since for this front-end, the
DWARF_CFA builtin is not registered. None of builtins.def are registered by the
way: how could I register only this one without duplicating code? Is it a good
idea anyway?

For the record, I bootstrapped and reg-tested this patch on x86_64-linux and
preliminary manual testing with a patched GDB[1] and Ada reproducers shows that
this approach is working.

Thoughts?

[1] This patch teaches GDB how to use DW_AT_static_link in order to find the
frame corresponding to the lexically enclosing scope. I think I will try to
submit it to GDB soon.