This is an archive of the discontinued LLVM Phabricator instance.

[WinEH] Validate funclet tokens in colorEHFunclets()
AbandonedPublic

Authored by sgraenitz on Oct 5 2022, 8:16 AM.

Details

Summary

When coloring EH funclets during WinEHPrepare, we should check that none of the involved funclet bundle operands refers to an invalid/out-of-scope funclet pad. This would lead to silent binary truncations later on.
For the time being users can demote the error into a warning by passing the -no-funclet-bundle-errors flag.

Diff Detail

Unit TestsFailed

Event Timeline

sgraenitz created this revision.Oct 5 2022, 8:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2022, 8:16 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
sgraenitz requested review of this revision.Oct 5 2022, 8:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2022, 8:16 AM

This came up in D134866. No upstream test case triggers the error. Clang reports the error/warning like this:

warning: linking module 'wineh-funclet-bundle-validation.ll': Invalid funclet token in BasicBlock catchret.dest (outside funclet)  %1 = catchpad within %0 [ptr null, i32 0, ptr null] [-Wlinker-warnings]
rnk added a comment.Oct 5 2022, 11:38 AM

I think it would be more reasonable to emit a diagnostic at the point where we mark the successor as implausible. This is an analysis function, which is called during instrumentation passes, and I don't think we should emit errors from an analysis helper.

llvm/lib/Analysis/EHPersonalities.cpp
111

What I had in mind was to essentially emit an error when the ColorVector has a size of more than one. That represents a situation where we would start cloning basic blocks in WinEHPrepare.

sgraenitz updated this revision to Diff 466093.Oct 7 2022, 9:10 AM

Move out of analysis pass and update for specific failure situation: verify that no funclet tokens exist outside of actual funclets (opt-in with -verify-funclet-tokens)

This is an analysis function, which is called during instrumentation passes, and I don't think we should emit errors from an analysis helper.

Fair point. I moved it into the first caller in WinEHPrepare.

What I had in mind was to essentially emit an error when the ColorVector has a size of more than one. That represents a situation where we would start cloning basic blocks in WinEHPrepare.

This is done already as a debug check in verifyPreparedFunclets() at the end of prepareExplicitEH().

It's not happening in our repro though! All basic blocks are assigned exactly one color. The catchret.dest block is marked implausible, because it refers to another color's funclet pad. As far as I can tell, this isn't verified anywhere. I adjusted the patch for this specific situation.

rnk added a comment.Oct 7 2022, 12:41 PM

This is an analysis function, which is called during instrumentation passes, and I don't think we should emit errors from an analysis helper.

Fair point. I moved it into the first caller in WinEHPrepare.

What I had in mind was to essentially emit an error when the ColorVector has a size of more than one. That represents a situation where we would start cloning basic blocks in WinEHPrepare.

This is done already as a debug check in verifyPreparedFunclets() at the end of prepareExplicitEH().

It's not happening in our repro though! All basic blocks are assigned exactly one color. The catchret.dest block is marked implausible, because it refers to another color's funclet pad. As far as I can tell, this isn't verified anywhere. I adjusted the patch for this specific situation.

Removing implausible terminators happens before that check, and it's supposed to make it so that the check passes and all blocks have a unique color. I guess what I'm suggesting is:

  • let's just delete, or flag off, this call to remove implausible terminators
  • always call verifyPreparedFunclets
  • replace the report_fatal_error calls with error diagnostics
sgraenitz abandoned this revision.Nov 16 2022, 5:08 AM

D138123 proposes to check funclet tokens in the IR Verifier