On Wed, 16 Nov 2022 08:48:51 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> Yes, I think it would be good to get a second review of the DWARF parser >> changes. Maybe @tstuefe? > >> Yes, I think it would be good to get a second review of the DWARF parser >> changes. Maybe @tstuefe? > > I'm a bit swamped, but try to take a look later today. I've pushed an update and changed the algorithm in the following way to address @tstuefe review comments: - Just read and ignore the filenames from DWARF which do not correspond to the one we are looking for (i.e. when `current_index != file_index`). - Reading the filename of interest (i.e. when `current_index == file_index`): - Read single chars and stop once the null terminator is found. - Reset buffer when file separator is found to skip the prefix path. - On `filename` buffer overflow: Keep reading, we could still be reading a path prefix and reset the buffer again when finding a file separator. - If filename does not fit into the provided buffer, use a generic overflow message `<OVERFLOW>`. If that does not fit either, use the minimal filename `L`. This allows to at least have the source information `L:line_no` which almost always is already enough to get to the actual source code location. Doing it in this way lets the parser succeed instead of failing. I've added some additional tests for the overflow scenarios and enforced `get_source_info()` to only accept buffers with a length of at least 2 to always allow the minimal filename `L`. Submitted some testing again in our CI (results look good so far) and additionally by running gtests with Clang slowdebug, fastdebug and release builds. I've done some additional manual testing, both with GCC and Clang builds. I've also played around by changing the buffer size in `vmError.cpp`. It works as expected: - buffer size 15, emitting `<OVERFLOW>` for filenames being too long: V [libjvm.so+0x8905fc] CompileWrapper::~CompileWrapper()+0x56 (compile.cpp:492) V [libjvm.so+0x8921e6] Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x1652 (compile.cpp:864) V [libjvm.so+0x77d171] C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x179 (c2compiler.cpp:113) V [libjvm.so+0x8b0fc4] CompileBroker::invoke_compiler_on_method(CompileTask*)+0x8e8 (<OVERFLOW>:2237) V [libjvm.so+0x8afc3d] CompileBroker::compiler_thread_loop()+0x3ed (<OVERFLOW>:1916) V [libjvm.so+0x8d047c] CompilerThread::thread_entry(JavaThread*, JavaThread*)+0x72 (<OVERFLOW>:58) V [libjvm.so+0xc63342] JavaThread::thread_main_inner()+0x144 (javaThread.cpp:699) V [libjvm.so+0xc631fa] JavaThread::run()+0x182 (javaThread.cpp:684) V [libjvm.so+0x1337633] Thread::call_run()+0x195 (thread.cpp:224) V [libjvm.so+0x10e38d7] thread_native_entry(Thread*)+0x19b (os_linux.cpp:710) - buffer size 2, emitting `L` for all filenames (being too long): V [libjvm.so+0x8905fc] CompileWrapper::~CompileWrapper()+0x56 (compile.cpp:492) V [libjvm.so+0x8921e6] Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x1652 (L:864) V [libjvm.so+0x77d171] C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x179 (L:113) V [libjvm.so+0x8b0fc4] CompileBroker::invoke_compiler_on_method(CompileTask*)+0x8e8 (L:2237) V [libjvm.so+0x8afc3d] CompileBroker::compiler_thread_loop()+0x3ed (L:1916) V [libjvm.so+0x8d047c] CompilerThread::thread_entry(JavaThread*, JavaThread*)+0x72 (L:58) V [libjvm.so+0xc6339c] JavaThread::thread_main_inner()+0x144 (L:699) V [libjvm.so+0xc63254] JavaThread::run()+0x182 (L:684) V [libjvm.so+0x1337693] Thread::call_run()+0x195 (L:224) V [libjvm.so+0x10e3937] thread_native_entry(Thread*)+0x19b (L:710) ------------- PR: https://git.openjdk.org/jdk/pull/10287
