On Wed, 19 Aug 2026 05:39:58 GMT, Yasumasa Suenaga <[email protected]> wrote:
> This is a part of [JDK-8382392](https://bugs.openjdk.org/browse/JDK-8382392). > > Debuginfo related code such as `open_debug_file()` have been implemented in > symtab.c. Maybe it would be used for symbol resolution so far. However it is > needed for reading `.debug_frame` section from ELF file. > > So this sub task moves debuginfo related code to salibelf.c because it > provides features for ELF. > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). src/jdk.hotspot.agent/linux/native/libsaproc/salibelf.c line 253: > 251: ELF_EHDR ehdr; > 252: read_elf_header(fd, &ehdr); > 253: ELF_SHDR* shbuf = read_section_header_table(fd, &ehdr); symtabl.c has already verified success of read_elf_header() and read_section_header_table() before calling open_debuginfo(), but a new user of open_debuginfo() may not. Should checks be made here also, or is there some implied contract that the caller has already made these checks. src/jdk.hotspot.agent/linux/native/libsaproc/salibelf.c line 254: > 252: read_elf_header(fd, &ehdr); > 253: ELF_SHDR* shbuf = read_section_header_table(fd, &ehdr); > 254: struct elf_section *scn_cache = (struct elf_section > *)calloc(ehdr.e_shnum, sizeof(struct elf_section)); Need to check for calloc failure. src/jdk.hotspot.agent/linux/native/libsaproc/salibelf.c line 258: > 256: scn_cache[cnt].c_shdr = &shbuf[cnt]; > 257: if (shbuf[cnt].sh_type == SHT_NOTE || shbuf[cnt].sh_type == > SHT_STRTAB) { > 258: scn_cache[cnt].c_data = read_section_data(fd, &ehdr, &shbuf[cnt]); Similar problem here. read_section_data() might return NULL, in which case we should abort. The caller of open_debuginfo() has already one this, but new callers may not. src/jdk.hotspot.agent/linux/native/libsaproc/salibelf.c line 267: > 265: // try again with build id. > 266: debug_fd = open_debuginfo_from_build_id(shbuf, &ehdr, scn_cache); > 267: } The order changed here. Can you explain why? I don't understand the significance of "build id" vs .gnu_debuglink. src/jdk.hotspot.agent/linux/native/libsaproc/salibelf.c line 271: > 269: // cleanup > 270: for (int cnt = 0; cnt < ehdr.e_shnum; cnt++) { > 271: if (shbuf[cnt].sh_type == SHT_NOTE) { What about cleanup of SHT_STRTAB? Why not just free all non-NULL entries? This will also catch sections found by find_section_by_name() instead of just found by sh_type. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/32434#discussion_r4065049042 PR Review Comment: https://git.openjdk.org/jdk/pull/32434#discussion_r4065050820 PR Review Comment: https://git.openjdk.org/jdk/pull/32434#discussion_r4065073327 PR Review Comment: https://git.openjdk.org/jdk/pull/32434#discussion_r4065198324 PR Review Comment: https://git.openjdk.org/jdk/pull/32434#discussion_r4065126991
