RE: [PATCH] Fix PR gdb/19208 - SIGSEV while opening Fortran program compiled with ifort

["Hahnfeld, Jonas" <Hahnfeld at itc dot rwth-aachen dot de>]

Hi,

first off: Your attached patch works for - sorry that Outlook killed my 
diff...

> -----Original Message-----
> From: Joel Brobecker [mailto:brobecker@adacore.com]
> Sent: Wednesday, January 06, 2016 7:20 AM
> To: Hahnfeld, Jonas
> Cc: gdb-patches@sourceware.org; qiyao@gcc.gnu.org
> Subject: Re: [PATCH] Fix PR gdb/19208 - SIGSEV while opening Fortran
> program compiled with ifort
>
> Hello,
>
> A lot of text below, but the summary is the following:
>
>   . The patch looks good but I couldn't apply it; I made the change
>     by hand, but can you confirm that it is correct by testing it
>     for me with ifort? If confirmed, I will push the change.
>
>   . Lots of advice (hence the amount of text). You might want
>     to take a look at our contribution checklist:
>     https://sourceware.org/gdb/wiki/ContributionChecklist
>     It'll help us during patch review, which in turn will help you
>     get your patches in faster :-)

Thanks for the pointer, I think this is neither linked at 
https://www.gnu.org/software/gdb/contribute/ nor in 
http://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/CONTRIBUTE;hb=HEAD

>
> Thanks for the contribution! Now, onto the details of the review...
>
> > Find attached a patch that fixes a SIGSEV for me when trying to open a
> > Fortran program compiled with ifort (using version 16.0.1.150).
> > The error can be reproduced with a most simple file only containing "end"
> > and no additional compiler flags.
> >
> > ---
> > diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 3d8923b..5890f78
> > 100644
> > --- a/gdb/ChangeLog
> > +++ b/gdb/ChangeLog
> > @@ -1,3 +1,8 @@
> > +2016-01-04  Jonas Hahnfeld  <Hahnfeld@itc.rwth-aachen.de>
> > +
> > +       * dwarf2read.c (read_partial_die): Fix PR gdb/19208 -
> > +       SIGSEV while opening Fortran program compiled with ifort
>
> Just a minor tweak on the ChangeLog entry: we put the PR number at the
> start of the entry, like this:
>
>        PR gdb/19208
>        * dwarf2read.c (read_partial_die): SIGSEV while opening
>        Fortran program compiled with ifort.

Ok, CONTRIBUTE mentions another convention in its last item.

>
> In terms of the change's description, there should be a period at the end of
> the sentence (added above). I would also describe the change, rather than
> what it prevents. Therefore:
>
>        PR gdb/19208
>        * dwarf2read.c (read_partial_die): Do not call set_objfile_main_name
>        if the function has no name.
>
> The part about the fact that it prevents a SIGSEGV and which compiler this
> was tested with is good information for the revision log. We normally try to
> put that kind of information in the code as much as we can, but the added
> part_die->name check is fairly obvious in this case.
>
> In terms of the patch itself, it does not apply to the current HEAD of the
> master branch, and I think this is because it got mucked up by your mailer
> (line folding, which I tried to fix by hand, and perhaps also spaces/tabs 
> being
> altered). Attached is the commit I intend to push, which I tested on x86_64-
> linux, but without Fortran support. Can you please let me know if it works 
> for
> you?

See above...

>
> In the future, to avoid this kind of issue, what would be nice is for you to 
> just
> create a commit on your end, and to either git-email it to us, or at least 
> attach
> the result of "git format-patch".

CONTRIBUTE explicitly mentions `git diff` which I only pasted into my mailer.
I just realized that the other mails were complete patches and git 
automatically generates the line `git diff` inside...

>
> Other than that, the patch looks good to me. I don't believe you have an FSF
> copyright assignement in place. We can take this patch as "tiny", but if you
> think you have other contributions coming, you might want to start the
> process now (this will also allow us to give you "write after approval" 
> access
> to the repo, allowing you to push your changes yourself once approved by
> one of the GDB maintainers.

Thanks, I was hoping for this being "tiny" and I currently don't plan to 
contribute larger patches. Will return to this point if necessary ;-)

Greetings
Jonas

>
> > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index
> > c410500..1020c12 100644
> > --- a/gdb/dwarf2read.c
> > +++ b/gdb/dwarf2read.c
> > @@ -15936,7 +15936,8 @@ read_partial_die (const struct
> > die_reader_specs *reader,
> >              compilers pick up the new representation, we'll support this
> >              practice.  */
> >           if (DW_UNSND (&attr) == DW_CC_program
> > -             && cu->language == language_fortran)
> > +             && cu->language == language_fortran
> > +             && part_die->name != NULL)
> >             set_objfile_main_name (objfile, part_die->name,
> > language_fortran);
> >           break;
> >         case DW_AT_inline:
> --
> Joel

Attachment: smime.p7s
Description: S/MIME cryptographic signature