This is an archive of the discontinued LLVM Phabricator instance.

Fix an assertion error when emitting call site info that combines two DW_OP_stack_values.
ClosedPublic

Authored by vsk on Mar 13 2020, 2:39 PM.

Details

Summary

When compiling

struct S {
  float w;
};
void f(long w, long b);
void g(struct S s) {
  int w = s.w;
  f(w, w*4);
}

I get Assertion failed: ((!CombinedExpr || CombinedExpr->isValid()) && "Combined debug expression is invalid").

That's because we combine two epxressions that both end in DW_OP_stack_value:

(lldb) p Expr->dump()
!DIExpression(DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_LLVM_convert, 64, DW_ATE_signed, DW_OP_stack_value)
(lldb) p Param.Expr->dump()
!DIExpression(DW_OP_constu, 4, DW_OP_mul, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_LLVM_convert, 64, DW_ATE_signed, DW_OP_stack_value)
(lldb) p CombinedExpr->isValid()
(bool) $0 = false
(lldb) p CombinedExpr->dump()
!DIExpression(4097, 32, 5, 4097, 64, 5, 16, 4, 30, 4097, 32, 5, 4097, 64, 5, 159, 159)

I believe that in this particular case combining two stack values is safe, but I didn't want to sink the special handlingi into DIExpression::append() because I do want everyone to think about what they are doing.

rdar://problem/60383095

Diff Detail

Event Timeline

aprantl created this revision.Mar 13 2020, 2:39 PM

I believe that that's the same assertion failure that @thakis found in Chromium today.

Can you also test on the reduced test cases in https://bugs.llvm.org/show_bug.cgi?id=45181 ?

dstenb added a subscriber: dstenb.Mar 13 2020, 3:26 PM

Can you also test on the reduced test cases in https://bugs.llvm.org/show_bug.cgi?id=45181 ?

Sorry for taking some extra time with that PR. This patch should fix that assertion, but we would emit invalid debug information, related to D76146, for that case.

Sorry for taking some extra time with that PR. This patch should fix that assertion, but we would emit invalid debug information, related to D76146, for that case.

But we already emit such debug information in other cases, so it's probably not a big issue.

djtodoro added inline comments.Mar 16 2020, 12:39 AM
llvm/test/DebugInfo/MIR/X86/callsite-stack-value.mir
2

Since the D73534 was reverted, this should be -debug-entry-values instead of -emit-debug-entry-values.

djtodoro added inline comments.Mar 16 2020, 3:08 AM
llvm/test/DebugInfo/MIR/X86/callsite-stack-value.mir
2

And I guess we can add --implicit-check-not=DW_TAG_GNU_call_site_parameter, since we are avoiding the production of call_site_params in this case.

Basically, looks good to me, but please post the patch with -U999999.

llvm/test/DebugInfo/MIR/X86/callsite-stack-value.mir
15

since we deleted the attributes, we can delete #0

19

as well as the #1

26

we can get rid of the (git@github.com:llvm/llvm-project 32e90cbcd19a83e20a86bfc1cbf7cec9729e9077) part

Sorry for taking some extra time with that PR. This patch should fix that assertion, but we would emit invalid debug information, related to D76146, for that case.

But we already emit such debug information in other cases, so it's probably not a big issue.

Since the patch (D76146) will solve the debug info issue in general, I guess this can go anyway?

vsk commandeered this revision.Mar 16 2020, 4:22 PM
vsk edited reviewers, added: aprantl; removed: vsk.

@djtodoro Perhaps I'm missing something, but it doesn't look like D76146 would address the issue of there being two DW_OP_stack_values in the combined expression. So I think something along the lines of this patch will be needed. I'll commandeer this, per Adrian's request.

Thanks for shepherding this patch, Vedant!

vsk updated this revision to Diff 250668.Mar 16 2020, 6:15 PM

Address review feedback.

djtodoro accepted this revision.Mar 17 2020, 12:12 AM
In D76164#1925521, @vsk wrote:

@djtodoro Perhaps I'm missing something, but it doesn't look like D76146 would address the issue of there being two DW_OP_stack_values in the combined expression. So I think something along the lines of this patch will be needed.

Sorry for not being clear enough, but I think we need this. I just wanted to confirm that fact, with the latest comment. :)

This revision is now accepted and ready to land.Mar 17 2020, 12:12 AM
In D76164#1925521, @vsk wrote:

@djtodoro Perhaps I'm missing something, but it doesn't look like D76146 would address the issue of there being two DW_OP_stack_values in the combined expression. So I think something along the lines of this patch will be needed.

Sorry for not being clear enough, but I think we need this. I just wanted to confirm that fact, with the latest comment. :)

And "the debug info issue" was referring to the issue regarding sign/zero exts debug info (David mentioned above), I did not want to point on this problem, so we are on the same page.

dstenb added inline comments.Mar 17 2020, 6:56 AM
llvm/test/DebugInfo/MIR/X86/callsite-stack-value.mir
6

Should this be a CHECK-NOT:?

djtodoro added inline comments.Mar 17 2020, 7:43 AM
llvm/test/DebugInfo/MIR/X86/callsite-stack-value.mir
6

+1

I would also prefer adding a check for not having a DW_TAG_call_site_parameter generated.

aprantl added inline comments.Mar 17 2020, 9:17 AM
llvm/lib/IR/DebugInfoMetadata.cpp
1116

typo: "concatenated" or "appended"?

vsk updated this revision to Diff 250831.Mar 17 2020, 11:07 AM

Fix typo, tighten up test.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2020, 12:58 PM