On Wed, 16 Aug 2023 11:45:54 GMT, Coleen Phillimore <cole...@openjdk.org> wrote:
>> Coleen Phillimore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Make elf fields 64 bits removing some static casts. > > src/hotspot/share/utilities/elfFile.cpp line 1426: > >> 1424: return false; >> 1425: } >> 1426: _state->_discriminator = static_cast<uint32_t>(discriminator); > > @chhagedorn These fields are declared as 32 bits but are read out of the > dwarf info as 64 bits. Should they be 64 bits? I just made them 64 bits. According to the DWARF 4 spec, the discriminator is an unsigned integer (i.e. 32 bits). Also, the other fields like `isa or `column` are unsigned integer. I think we would be good to keep them as unsigned integer to follow the spec. `read_uleb128()` reads up to 8 bytes and just returns the result in a `uint64_t`. But sometimes, it should only read 1, 2, or 4 bytes. In that case, we additionally specify `check_size` to ensure that we can safely cast it. I guess we could change these calls to `read_uleb128()` to return a more precise type instead of relying on `check_size` passed in by a caller + casting the result in the caller. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15233#discussion_r1295814369
