On Wed, 16 Nov 2022 11:59:19 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> Christian Hagedorn has updated the pull request with a new target base due >> to a merge or a rebase. The incremental webrev excludes the unrelated >> changes brought in by the merge/rebase. The pull request contains six >> additional commits since the last revision: >> >> - Always read full filename and strip prefix path and only then cut >> filename to fit output buffer >> - Merge branch 'master' into JDK-8293422 >> - Merge branch 'master' into JDK-8293422 >> - Review comments from Thomas >> - Change old bailout fix to only apply to Clang versions older than 5.0 and >> add new fix with -gdwarf-aranges + -gdwarf-4 for Clang 5.0+ >> - 8293422: DWARF emitted by Clang cannot be parsed > > src/hotspot/share/utilities/elfFile.cpp line 1605: > >> 1603: uint32_t current_index = 1; // file_names start at index 1 >> 1604: const size_t dwarf_filename_len = 1024; >> 1605: char dwarf_filename[dwarf_filename_len]; // Store the filename read >> from DWARF which is then copied to 'filename'. > > Putting such a large array on the stack is a bit borderline. Especially in > error reporting, where you may build up stack repeatedly via secondary crash > handling. I realize though that no good alternatives exist. C-heap may be > corrupted, ResourceArea is also out of the question. Could we get away with > using filename directly? That's true. Maybe I can change the algorithm to read single characters instead and reset the buffer once I'm encountering a file separator. I'll try that out. > Why uint16_t? There is no specific reason. I'll change it to `uint32_t`. > We have `pointer_delta()` for that btw if you want to be super correct. See > globalDefinitions.hpp Ah that's great! Will use that one instead. > 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? ------------- PR: https://git.openjdk.org/jdk/pull/10287
