On Fri, 16 Oct 2026 22:14:50 GMT, Chris Plummer <[email protected]> wrote:
>> Yasumasa Suenaga has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Close library FD when lib_info is destroyed
>> - Consider 64bit DWARF
>
> src/jdk.hotspot.agent/linux/native/libsaproc/dwarf.cpp line 123:
>
>> 121: // address_size and segment_size are located in DWARF 4 or later,
>> 122: // but they are not available in .eh_frame defined by Linux LSB Core
>> Generic 5.0.
>> 123: if (!_lib->frame.is_debug_frame && _dwarf_version >= 4) {
>
> This is suppose to check for extra fields in DWARF 4 or later, but your
> !_lib->frame.is_debug_frame check is backwards. The "!" should be removed.
Thanks, fixed.
> src/jdk.hotspot.agent/linux/native/libsaproc/dwarf.cpp line 374:
>
>> 372: if (length == 0L) {
>> 373: break; // it means "terminator" in .eh_frame, so go through to
>> .debug_frame
>> 374: } else if (_buf + length > end) {
>
> You need a check before calling get_entry_length(). Otherwise if _buf == end,
> get_entry_length will read beyond the end of the buffer. I think changing
> the loop to check for "buf < end" is the proper fix.
I updated loop condition in new commit.
> src/jdk.hotspot.agent/linux/native/libsaproc/dwarf.cpp line 375:
>
>> 373: break; // it means "terminator" in .eh_frame, so go through to
>> .debug_frame
>> 374: } else if (_buf + length > end) {
>> 375: // unexpected: DWARF is broken?
>
> Should this print a warning?
>
> Is this unexpected because you expect to find the PC in the frame record
> before reaching the end?
Yes.
DwarfParser expects to find appropriate FDE before reaching end of buffer. I
added warning message in new commit.
> src/jdk.hotspot.agent/linux/native/libsaproc/dwarf.cpp line 439:
>
>> 437: int debug_fd = open_debuginfo(_lib->name, _lib->fd);
>> 438: if (debug_fd != -1 && read_frame(".debug_frame", debug_fd,
>> &frame)) {
>> 439: close(debug_fd);
>
> The close() should be done even if read_frame() fails to find the
> .debug_frame. If the open succeeded, then you need to do the close.
You are right. Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/33375#discussion_r4301100249
PR Review Comment: https://git.openjdk.org/jdk/pull/33375#discussion_r4301099783
PR Review Comment: https://git.openjdk.org/jdk/pull/33375#discussion_r4301098175
PR Review Comment: https://git.openjdk.org/jdk/pull/33375#discussion_r4301100903
