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

[Simon Marchi <simark at simark dot ca>]

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