Page MenuHomePhabricator

[CodeExtractor] Erase debug intrinsics in outlined thunks
ClosedPublic

Authored by vsk on Oct 14 2018, 11:20 PM.

Details

Summary

Variable updates within the outlined function are invisible to
debuggers. This could be improved by defining a DISubprogram for the
new function. For the moment, simply erase the debug intrinsics instead.

This fixes verifier failures about function-local metadata being used in
the wrong function, seen while testing the hot/cold splitting pass.

rdar://45142482

Diff Detail

Repository
rL LLVM

Event Timeline

vsk created this revision.Oct 14 2018, 11:20 PM

What kind of DISubprogram do we assign to the outlined function now?

vsk added a comment.Oct 15 2018, 9:16 AM

What kind of DISubprogram do we assign to the outlined function now?

Currently we don't assign one.

aprantl accepted this revision.Oct 15 2018, 10:02 AM

That's not great...

This revision is now accepted and ready to land.Oct 15 2018, 10:02 AM

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

vsk added a comment.Oct 15 2018, 11:15 AM

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

It does. I'm not sure that there's extra value in including the attached test case -- it uses an old textual IR format that's hard to update, and reduces to something similar to the test in this patch.

Fwiw I've tested this patch by building several internal frameworks and running Csmith over the weekend. I found no regressions. With Csmith, the testing method was to run -Os -g -mllvm -hot-cold-split={true, false} and check that the two CRCs were the same.

In D53267#1265669, @vsk wrote:

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

It does. I'm not sure that there's extra value in including the attached test case -- it uses an old textual IR format that's hard to update, and reduces to something similar to the test in this patch.

Fwiw I've tested this patch by building several internal frameworks and running Csmith over the weekend. I found no regressions. With Csmith, the testing method was to run -Os -g -mllvm -hot-cold-split={true, false} and check that the two CRCs were the same.

Thanks for the fix. I didn't see this patch before I updated that bug. This should fix my issue, which related to some llvm.dbg.value intrinsics in outlined code that weren't updated properly.

In D53267#1265669, @vsk wrote:

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

It does. I'm not sure that there's extra value in including the attached test case -- it uses an old textual IR format that's hard to update, and reduces to something similar to the test in this patch.

Fwiw I've tested this patch by building several internal frameworks and running Csmith over the weekend. I found no regressions. With Csmith, the testing method was to run -Os -g -mllvm -hot-cold-split={true, false} and check that the two CRCs were the same.

Thanks for the fix. I didn't see this patch before I updated that bug. This should fix my issue, which related to some llvm.dbg.value intrinsics in outlined code that weren't updated properly.

I should add that in the one case I looked at, the first argument to the outlined llvm.dbg.value was not updated correctly, which seems a little different than the failure mode here. So there might be multiple issues with these outlined intrinsics that need fixing. Not sure if you want to update the comment.

vsk added a comment.Oct 15 2018, 11:24 AM
In D53267#1265669, @vsk wrote:

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

It does. I'm not sure that there's extra value in including the attached test case -- it uses an old textual IR format that's hard to update, and reduces to something similar to the test in this patch.

Fwiw I've tested this patch by building several internal frameworks and running Csmith over the weekend. I found no regressions. With Csmith, the testing method was to run -Os -g -mllvm -hot-cold-split={true, false} and check that the two CRCs were the same.

Thanks for the fix. I didn't see this patch before I updated that bug. This should fix my issue, which related to some llvm.dbg.value intrinsics in outlined code that weren't updated properly.

I should add that in the one case I looked at, the first argument to the outlined llvm.dbg.value was not updated correctly, which seems a little different than the failure mode here. So there might be multiple issues with these outlined intrinsics that need fixing. Not sure if you want to update the comment.

Hm, I think that's the same failure mode I saw. A dbg.value from the original function was moved to the outlined function, but its ValueAsMetadata operand (the "function-local metadata") wasn't updated to point to an Argument in the outlined function. The error I'm referring to comes from Verifier::visitValueAsMetadata.

In D53267#1265681, @vsk wrote:
In D53267#1265669, @vsk wrote:

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

It does. I'm not sure that there's extra value in including the attached test case -- it uses an old textual IR format that's hard to update, and reduces to something similar to the test in this patch.

Fwiw I've tested this patch by building several internal frameworks and running Csmith over the weekend. I found no regressions. With Csmith, the testing method was to run -Os -g -mllvm -hot-cold-split={true, false} and check that the two CRCs were the same.

Thanks for the fix. I didn't see this patch before I updated that bug. This should fix my issue, which related to some llvm.dbg.value intrinsics in outlined code that weren't updated properly.

I should add that in the one case I looked at, the first argument to the outlined llvm.dbg.value was not updated correctly, which seems a little different than the failure mode here. So there might be multiple issues with these outlined intrinsics that need fixing. Not sure if you want to update the comment.

Hm, I think that's the same failure mode I saw. A dbg.value from the original function was moved to the outlined function, but its ValueAsMetadata operand (the "function-local metadata") wasn't updated to point to an Argument in the outlined function.

Yeah, sounds like it.

The error I'm referring to comes from Verifier::visitValueAsMetadata.

Yep, I saw that one when building with hot cold splitting and -g. Originally the failure I saw was different, because it manifests differently when building with ThinLTO, where essentially the bitcode gets corrupted because the operand types don't match.

vsk added a comment.Oct 15 2018, 11:42 AM
In D53267#1265681, @vsk wrote:
In D53267#1265669, @vsk wrote:

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

It does. I'm not sure that there's extra value in including the attached test case -- it uses an old textual IR format that's hard to update, and reduces to something similar to the test in this patch.

Fwiw I've tested this patch by building several internal frameworks and running Csmith over the weekend. I found no regressions. With Csmith, the testing method was to run -Os -g -mllvm -hot-cold-split={true, false} and check that the two CRCs were the same.

Thanks for the fix. I didn't see this patch before I updated that bug. This should fix my issue, which related to some llvm.dbg.value intrinsics in outlined code that weren't updated properly.

I should add that in the one case I looked at, the first argument to the outlined llvm.dbg.value was not updated correctly, which seems a little different than the failure mode here. So there might be multiple issues with these outlined intrinsics that need fixing. Not sure if you want to update the comment.

Hm, I think that's the same failure mode I saw. A dbg.value from the original function was moved to the outlined function, but its ValueAsMetadata operand (the "function-local metadata") wasn't updated to point to an Argument in the outlined function.

Yeah, sounds like it.

The error I'm referring to comes from Verifier::visitValueAsMetadata.

Yep, I saw that one when building with hot cold splitting and -g. Originally the failure I saw was different, because it manifests differently when building with ThinLTO, where essentially the bitcode gets corrupted because the operand types don't match.

I think I know what you're talking about. I saw something similar while building an internal framework with -flto=thin: with the verifier disabled, it manifested as an assertion failure in the bitcode writer's ValueEnumerator. But the underlying issue is the same as the one diagnosed by the verifier.

This revision was automatically updated to reflect the committed changes.