Add rudimentry debug info metadata emission #4225
Conversation
This adds just the debug info metadata for Compilation Units (the top level container of debug info) - but without anything in them, LLVM won't emit them at all, so while this is testable at the IR level, it isn't observable at the object level until more debug info is added.
There was a problem hiding this comment.
Looks good! Thank you for the contributions. Just a few style comments.
Are you using pre-commit (pre-commit install)? I think it would've automatically fixed this by running clang-format for you.
toolchain/lower/file_context.cpp
Outdated
| CARBON_CHECK(!sem_ir.has_errors()) | ||
| << "Generating LLVM IR from invalid SemIR::File is unsupported."; | ||
| BuildCompileUnit(module_name); |
There was a problem hiding this comment.
Maybe rename this to BuildDICompileUnit? I was having trouble seeing where di_compile_unit_ was initialized -- but maybe it'd be even better to have the method be static and return the DICompileUnit* instead of void, allowing initializing as di_compile_unit_(BuildDICompileUnit(module_name))?
There was a problem hiding this comment.
Sure, renamed to BuildDICompileUnit.
It'd take a few more parameters to make it static (include_debug_info_ (though we could move that out into the call site), di_builder_, and llvm_module_)) - I'll give it a go and you can see whether it feels better that way.
There was a problem hiding this comment.
This looks good, I think one nice thing about static is it's harder to accidentally use uninitialized members.
Co-authored-by: Carbon Infra Bot <[email protected]>
Co-authored-by: Carbon Infra Bot <[email protected]>
Co-authored-by: Jon Ross-Perkins <[email protected]>
(also rename it, adding the 'DI' in the name to match the type and member variable name) This renders `include_debug_info_` unused, so remove it - we can rely on the CU being non-null as the marker for debug info emission in future patches/work.
Ah, nope - missed that, sorry. Thanks for the pointer/reminder. Think I managed to run it on some of my stuff that's already committed (with |
Not a comment on the patch, but maybe update the description before merging to summarize what your suggested starting point is for these questions, and what motivates that starting point? (FWIW, not disagreeing with the current behavior, mostly looking to have a record of it.) |
Updated - hopefully that's something along the lines of what you're looking for. |
Head branch was pushed to by a user without write access
This adds just the debug info metadata for Compilation Units (the top level container of debug info) - but without anything in them, LLVM won't emit them at all, so while this is testable at the IR level, it isn't observable at the object level until more debug info is added.
A couple of starting points in this patch:
--debug-info, seems to match the naming/style of other flags in the carbon driver, though this is different from the naming conventions of clang/gcc) that enables debug info when lowering. Open to other names/approaches (on by default? historically debug info's been to large/expensive to do this, so sticking with that precedent for now).