Re: [PATCH v2 1/4] DWARF 5 support: Handle dwo_id

["Ali Tamur via gdb-patches" <gdb-patches@sourceware.org>]

Thank you for the review, I am submitting now.

On Mon, Sep 9, 2019 at 2:21 PM Simon Marchi <simark@simark.ca> wrote:
>
> On 2019-09-09 2:58 p.m., Ali Tamur via gdb-patches wrote:
> >> Since you're touching the split-DWARF code, it might be a good idea to run the whole
> >> testsuite with -gsplit-dwarf, with DWARF 4 (to verify there are no regressions) and
> >> DWARF 5 (to check how things improve).
> > Done. Both with and without -gsplit-dwarf the set of tests that fail is
> > identical. This is expected as unless I introduced a bug, there should be no
> > behavioral change at all for DWARF 4. I also ran the testsuite with -gdwarf-5
> > and the number of failures went down from 32687 to 1163, but until gdb can
> > handle 'hello world' (hopefully at the end my patch series) I think that is
> > not a very meaningful metric.
>
> You're right that it's not very meaningful, but still encouraging :).
>
> An interesting one will be to compare the results for DWARF 4 vs for DWARF 5.
>
> >> And since DWARF 5 is relatively new stuff, results can vary greatly if using different versions of the
> >> same compiler to run the tests.  So if you could mention in the commit message which gcc version the
> >> tests were ran against, I think it would be useful.
> > Updated the commit message to include the gcc version (8.3.0).
>
> Thanks.
>
> > Sorry for the style mistakes; I am accustomed to a different style and also
> > have become too dependent on clang-tidy.
>
> No problem, and I completely understand.  I had a taste of clang-format (I
> presume you meant clang-format) for C++ and black for Python, and it's really
> nice not to have to think about formatting.  It would be wonderful to have
> something equivalent here :).
>
> I just found one last little thing:
>
> > gdb/ChangeLog:
> >
> >         * gdb/dwarf2read.c (comp_unit_head): Update comment.
>
> The file name here should just be "dwarf2read.c", as it should be relative to
> the ChangeLog file the entry is in.
>
> The patch LGTM with that fixed, you can go ahead and push.  Thanks for following
> up quickly and efficiently on review comments.
>
> I'd like to take a look at the other patches of the series, but that won't be
> before at least next week, as I'll be quite busy with the GNU Cauldron Conference
> until then.  But if somebody else wants to review them, please go ahead.
>
> Simon