feat: Support GDB index#1990
Conversation
| fallback_cu: u32, | ||
| mut on_entry: impl FnMut(&'data [u8], u32), | ||
| ) { | ||
| for section_name in [".debug_gnu_pubnames", ".debug_gnu_pubtypes"] { |
There was a problem hiding this comment.
The symbol table construction assumes the presence of these two sections by adding the compiler flag -ggnu-pubnames (see the added integration test). Without these sections, the symbol table will not be built and only the CU list and address table will be constructed. This effectively means you won't benefit from GDB index, but since lld and mold behave this way, I've aligned the implementation.
There was a problem hiding this comment.
Can we emit a warning to the user telling the option is leading to a GDB index result?
|
I did I have no idea what happened... 🤷 |
marxin
left a comment
marxin
left a comment
There was a problem hiding this comment.
Given the numerous findings I created - can you explain what was the genesis of the PR and how much was a LLM involved? I know we had a PR for the very same functionality that was closed right after it was opened. Is it an incarnation of the changes, or was it created completely independently?
| 4 + init_len as usize | ||
| }; | ||
| if total == 0 || offset + total > data.len() { | ||
| break; |
There was a problem hiding this comment.
Shouldn't we rather report a warning as the emitted GDB index will be likely incomeplete/corrupted?
|
|
||
| // Emit into the output buffer. | ||
| let total = cp_off as usize + cv_data.len() + str_data.len(); | ||
| let len = buf.len().min(total); |
There was a problem hiding this comment.
This should be a hard error if the allocated buffer does not match the expected written data.
|
@marxin (I haven't seen the review details yet)
I think you mean #1605. I used that as a reference when implementing this, since I had also reviewed it at the time and therefore already had some familiarity with the surrounding context and implementation details. However, I did not reuse it directly for licensing concerns (and in any case, it does not support version 9 as-is). I mainly used LLMs to help refine the implementation through review and iterative revisions. |
Yes - fine, that's a good approach you chose ;)
All good! Let's iterate on the PR, given you knowledge, I guess you can address the findings pretty fast. Happy to review and help you with the feature. |
| let (header_size, set_end, debug_info_offset) = if init_len == 0xFFFF_FFFF { | ||
| // DWARF64: 4 + 8(len) + 2(ver) + 8(offset) + 8(size) = 30 | ||
| if pos + 30 > data.len() { | ||
| break; |
There was a problem hiding this comment.
I believe most of the breaks in the function should be actually replaced with an error handling instead.
There was a problem hiding this comment.
As shown by this test failure, some toolchains don't generate strictly correct .debug_info. Therefore, both lld and mold are designed to be tolerant of such structures, so the original break approach should work well I think.
2 participants
This patch adds support for
--gdb-indexand--no-gdb-index. Although we support version 9 here, the test environment may produce a version 7 GDB index when using lld. This patch also adds a test that checks whether specific symbols are included in the symbol table of the.gdb_indexsection. Because of this requirement, the integration test includes logic to support versions earlier than version 9 as well.Closes #811