Add line-level debug info #4247
Conversation
Seems to work with lldb ( https://pastebin.com/igKkNECm ), though gdb has /some/ trouble with the paths (they aren't complete - just using the filename directly, not providing the working directory - might be some quick hacks that can help there).
dwblaikie
force-pushed
the
debug_info_source_locations
branch
from
11ff907
to
2e90a52
Compare
| builder_.SetCurrentDebugLocation( | ||
| llvm::DILocation::get(builder_.getContext(), loc.line_number, | ||
| loc.column_number, di_subprogram_)); |
There was a problem hiding this comment.
Just to be sure, this is assuming the locations are valid and in the current file. Should that be checked? For example, loc.line_number <= 0 or loc.column_number <= 0 would indicate unknown. You can see some of this in FormatSnippet
Doing something on the file could just be a TODO.
Not sure how much we test multi-file lowering today, but might still be worth adding some resilience and/or todos.
There was a problem hiding this comment.
Hmm - 0 is an OK value for the debug info (it means "we can't assign this instruction to any particular source line") and I think we use 0 for invalid column number too. Is it just 0 for invalid, or are really <= 0? (carrying some extra information about kinds of invalidity or something in this?)
What causes invalid values but successful code generation? (happy to add some test coverage)
The multi-file issue is due to the unimpleented/trivially implemented context callback passed to the ConvertLoc call?
There was a problem hiding this comment.
Hmm - 0 is an OK value for the debug info (it means "we can't assign this instruction to any particular source line") and I think we use 0 for invalid column number too. Is it just 0 for invalid, or are really <= 0? (carrying some extra information about kinds of invalidity or something in this?)
What causes invalid values but successful code generation? (happy to add some test coverage)
-1 is used for "unknown", it's the default value for both (https://github.com/carbon-language/carbon-lang/blob/trunk/toolchain/diagnostics/diagnostic.h#L61-L64).
The default value is sometimes used, for example here:
https://github.com/carbon-language/carbon-lang/blob/trunk/toolchain/parse/tree_node_diagnostic_converter.h#L48-L51
I know this comes up in check:
https://github.com/search?q=repo%3Acarbon-language%2Fcarbon-lang+path%3Atestdata%2F+%22.carbon%3A+%22&type=code
I'm not sure what test coverage would look like. However, it might still be good to handle it since it's allowed by the API call.
The multi-file issue is due to the unimpleented/trivially implemented context callback passed to the ConvertLoc call?
No, this is just what the location converter is up to. A location can be either a node_id or a import_ir_id. In the latter case, it's referencing an imported instruction. Again, not sure it'll happen right now. OTOH, probably will start coming up as complexity increases, e.g. for template expansions or maybe inlining code.
Not sure why you're looking at the context though... To be sure, the DiagnosticLoc has a field "filename" for this. You'd need to ensure that everything you're using is a Parse::Node in order to be guaranteed it's in the file. e.g., something like:
auto loc_id = insts().GetLocId(inst_id);
if (loc_id.is_valid() && loc_id.is_node_id()) {
// This is in the current file.
}
There was a problem hiding this comment.
Note, an option is to just leave a guardrail, e.g.:
CARBON_CHECK(loc.filename == sem_ir.filename() && loc.line_number >= 1 && loc.column_number >= 1) << "We currently only support lowering instructions in the current file";
There was a problem hiding this comment.
Note, you could also look at changing line_number to be 0-defaulted, to help DebugInfo. I think I'd chosen -1 in order to avoid accidents with 0 versus 1 as the first correct value: although comments don't specify, I think 1 is where it starts, but a bug could lead to 0 and it'd stick out a little more.
But if you change that, do add comments specifying why the values are chosen that way.
There was a problem hiding this comment.
FYI, line: 4294967295 does appear in the .carbon testdata files as a result of this change.
There was a problem hiding this comment.
Thanks for pointing that out, I hadn't been looking the right way to notice that.
|
I'm going to go ahead and merge since this is basically good, and send a separate PR for the safety check. |
Seems to work with lldb ( https://pastebin.com/igKkNECm ), though gdb has /some/ trouble with the paths (they aren't complete - just using the filename directly, not providing the working directory - might be some quick hacks that can help there).