RFR: 8242181: [Linux] Show source information when printing native stack traces in hs_err files [v4]

[Christian Hagedorn]

On Tue, 22 Feb 2022 09:59:36 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Make dwarf tag NOT_PRODUCT
>
> src/hotspot/share/utilities/elfFile.cpp line 319:
> 
>> 317:     }
>> 318:     log_develop_info(dwarf)("No separate .debuginfo file for library %s. It already contains the required DWARF sections.", _filepath);
>> 319:     _dwarf_file = new (std::nothrow) DwarfFile(_filepath);
> 
> Would it be useful to explicitly bail out on a `nullptr` value here to avoid crashes below?

Yes, I think that's the right way. I changed other allocations as well to bail out.

> src/hotspot/share/utilities/elfFile.cpp line 357:
> 
>> 355:   }
>> 356: 
>> 357:   strcpy(debug_pathname, _filepath);
> 
> I'm always a bit uneasy using "raw" `strcpy` instead of `strncpy` and friends. The code seems to be correct though.

Yes that's true. I updated usages while introducing a new helper class `DwarfFilePath`.

> src/hotspot/share/utilities/elfFile.cpp line 784:
> 
>> 782:   }
>> 783: 
>> 784:   if (!_reader.read_byte(&_header._address_size) || NOT_LP64(_header._address_size != 4)  LP64_ONLY( _header._address_size != 8)) {
> 
> Since this is the second time for the clause `|| NOT_LP64(_header._address_size != 4) LP64_ONLY( _header._address_size != 8)` maybe it is useful to make a constant out of the accepted address size somewhere instead of repeating this over and over.
> It's value could even be something like `sizeof(intptr_t)` or so.

I agree, I introduced a new constant `DwarfFile::ADDRESS_SIZE`.

> src/hotspot/share/utilities/elfFile.cpp line 1070:
> 
>> 1068:     // reason, GCC is currently using version 3 as specified in the DWARF 3 spec for the line number program even though GCC should
>> 1069:     // be using version 4 for DWARF 4 as it emits DWARF 4 by default.
>> 1070:     return false;
> 
> According to the specification (pg112):
> 
>> `version (uhalf)`
>> A version number (see Appendix F). This number is specific to the line number information
>> and is independent of the DWARF version number.
> 
> So this is just fine - actually things may break if the code accepted version 4 here assuming that there are breaking differences.
> On the other hand Appendix F mentions that DWARF4 contains .debug_line information in version 4.

The `LineNumberProgram` class should be able to handle both version 3 and 4. There are some differences (see `_dwarf_version` checks). But I found that GCC even mixes version 3 and 4:
https://github.com/chhagedorn/jdk/blob/820f0da65ab06b28ac75eec96d35269addda0246/src/hotspot/share/utilities/elfFile.cpp#L1302-L1308

> src/hotspot/share/utilities/elfFile.hpp line 211:
> 
>> 209: 
>> 210:   // Load the DWARF file (.debuginfo) that belongs to this file.
>> 211:   bool load_dwarf_file();
> 
> It would be nice to summarize from which places this methods tries to load the debug info to prevent the need for digging for it in the method implementation.

Good suggestion. I added a summary and refactored the different loading attempts into separate methods together with a new class `DwarfFilePath` which makes it easier to prepare the different paths.

-------------

PR: https://git.openjdk.java.net/jdk/pull/7126