On Tue, 22 Feb 2022 09:59:36 GMT, Thomas Schatzl <tscha...@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
