On Wed, 16 Nov 2022 15:50:31 GMT, Christian Hagedorn <chaged...@openjdk.org> wrote:
>> src/hotspot/share/utilities/elfFile.cpp line 1651: >> >>> 1649: int bytes_written = jio_snprintf(dst, count, "%s", src); >>> 1650: // Add null terminator. >>> 1651: dst[count - 1] = '\0'; >> >> Does it make sense to return a truncated file name up to the caller of >> `DwarfFile::LineNumberProgram::get_filename_from_header()`? Will this not >> just confuse him? I think it makes more sense to cleanly handle truncation, >> and e.g. skip file parsing for dwarf files with too long names. > > I think you're right that it's rather unexpected to get an incomplete > filename back. But just silently skipping the filename might be confusing as > well. We could either print an error (I guess that's useful either way but > should only be printed with `TraceDwarfLevel`) or just return a generic > "buffer overflow" string as filename instead if it fits into the provided > filename buffer. And only if that's not possible we could silently skip the > filename. Would that be an option? Optional tracing and returning a generic string sounds fine. >> src/hotspot/share/utilities/elfFile.hpp line 865: >> >>> 863: bool get_filename_from_header(uint32_t file_index, char* filename, >>> size_t filename_len); >>> 864: static void strip_path_prefix(char* filename, const size_t >>> filename_len); >>> 865: static void copy_dwarf_filename_to_filename(char* src, size_t >>> src_len, char* dst, size_t dst_len); >> >> Stupid question, do these have to be exposed? Or could they be just static >> helpers in elfFile.cpp? > > In theory, they don't need to be exposed in the sense of being declared in > the header. But in terms of readability, I've decided to put them in the > private block of class `LineNumberProgram` which is the only user of these > methods. What would be the advantages of moving them completely to > `elfFile.cpp` as static helper functions? Oh, I generally just prefer to keep things locally if possible. Slim interfaces, less polluted global namespace, possibly (though not here) less include deps. Don't worry, if you prefer it this way, keep it in. ------------- PR: https://git.openjdk.org/jdk/pull/10287
