[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-dev
Subject:    Re: RFR: 8242181: [Linux] Show source information when printing native stack traces in hs_err files 
From:       Christian Hagedorn <chagedorn () openjdk ! java ! net>
Date:       2022-02-28 16:22:31
Message-ID: 5YeksetlUoja6cRgWtaorVsXCDLEQRw8s7B9W5UJOUE=.22f23a33-8877-4c3a-9b6d-f648fb1c4fc3 () github ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]