debug-info: Add checks for instructions without source locations #4251
Conversation
I think this is what you were looking for, @jonmeow? (I was trying to figure out how to tersely phrase the check to be "this isn't supported here yet, and not clear if it needs to be supported here - but if this assert is hit, then we should add support here?" - open to wording suggestions) (this will fail testing due to carbon-language#4249 - so shouldn't be submitted before that, though they aren't textually/patch-ually dependent on each other)
|
@dwblaikie Can you please fix your commit message to not @ me? I will already get notifications from PRs, I don't need notifications from individual commits too. i.e., please fix: dwblaikie@8134dfb I would generally encourage using PR comments to @ people rather than commits, since the PR is where discussion will occur. |
|
BTW, per josh11b at #4247 (comment), I think we can't just add these CHECKs and the line number issue I was noting is already a problem. There's more going on here so I'm poking around. |
| CARBON_CHECK(loc.filename == di_subprogram_->getFile()->getFilename()) | ||
| << "Instructions located in a different file from their enclosing " | ||
| "function aren't handled yet"; | ||
| CARBON_CHECK(loc.line_number >= 1 && loc.column_number >= 1) | ||
| << "Instructions without line locations aren't handled yet"; |
There was a problem hiding this comment.
FYI, I think this isn't catching the existing issues because it's not used everywhere GetDiagnosticLoc is.
There was a problem hiding this comment.
To be sure, what I was noting here was that file_context.cpp also queries ConvertLoc. Since it won't use this code, the function missing a line number could be missed. That is, this would miss:
// CHECK:STDOUT: !4 = distinct !DISubprogram(name: "__global_init", scope: null, file: !3, line: 4294967295, type: !5, spFlags: DISPFlagDefinition, unit: !2)
#4252 fixes this.
|
FYI, #4252 with a different approach on the line numbers, which looks like it fixes the current issues. |
Works for me - I'll abandon/delete this change. Thanks! |
Ah, yeah - sorry, old habit from LLVM where commit message -> description in Phabricator I guess, without the extra layer of commits nested within a review. Will try to avoid writing review descriptions in commits in the future. |
Thanks! Other aspect of this with GitHub, I would've also gotten a notification when the commit was merged. I really wish GH gave me a way of narrowly disabling that. |
Mainly, changes the default from -1 to 0 in DiagnosticLoc, still trying to keep reusing that. Nothing except for the lowered output is affected, so I think this is fine. Also, have lowering consistently call GetDiagnosticLoc. Pulls in one of the CHECKs suggested from #4251 Co-authored-by: David Blaikie <[email protected]>
Mainly, changes the default from -1 to 0 in DiagnosticLoc, still trying to keep reusing that. Nothing except for the lowered output is affected, so I think this is fine. Also, have lowering consistently call GetDiagnosticLoc. Pulls in one of the CHECKs suggested from carbon-language#4251 Co-authored-by: David Blaikie <[email protected]>
Mainly, changes the default from -1 to 0 in DiagnosticLoc, still trying to keep reusing that. Nothing except for the lowered output is affected, so I think this is fine. Also, have lowering consistently call GetDiagnosticLoc. Pulls in one of the CHECKs suggested from carbon-language#4251 Co-authored-by: David Blaikie <[email protected]>
I think this is what you were looking for, @jonmeow? (I was trying to figure out how to tersely phrase the check to be "this isn't supported here yet, and not clear if it needs to be supported here - but if this assert is hit, then we should add support here?" - open to wording suggestions)
(this will fail testing due to #4249 - so shouldn't be submitted before that, though they aren't textually/patch-ually dependent on each other)