Page MenuHomePhabricator

[Windows EH] Fix the order of Nested try-catches in $tryMap$ table
AbandonedPublic

Authored by asmith on Apr 10 2020, 4:06 PM.

Details

Summary

This bug is exposed by Test7 of ehthrow.cxx in MSVC EH suite where
a rethrow occurs in a try-catch inside a catch (i.e., a nested Catch
handlers). See the test code in
https://github.com/microsoft/compiler-tests/blob/master/eh/ehthrow.cxx#L346

When an object is rethrown in a Catch handler, the copy-ctor of this
object must be executed after the destructions of live objects, but
BEFORE the dtors of live objects in parent handlers.

Today Windows runtime (__CxxFrameHandler3 & 4) expects nested Catch handers
are stored in pre-order (outer first, inner next) in $tryMap$ table, so
that given a State, its Catch's beginning State can be properly
retrieved. The Catch beginning state (which is also the ending State) is
the State where rethrown object's copy-ctor must take place.

LLVM currently stores nested catch handlers in post-ordering because
it's the natural way to compute the highest State in Catch.
The fix is to simply store TryCatch handler in pre-order, but update
Catch's highest State after child Catches are all processed.

Diff Detail

Event Timeline

asmith created this revision.Apr 10 2020, 4:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2020, 4:06 PM
asmith updated this revision to Diff 256702.Apr 10 2020, 4:39 PM
asmith retitled this revision from [Windows EH] the order of Nested try-catches in $tryMap$ table to [Windows EH] Fix the order of Nested try-catches in $tryMap$ table.

Nit - update comment

rnk added a comment.Apr 15 2020, 2:11 PM

Sounds good, and the fix looks simple, but please add a test.

I'm surprised this change doesn't break an existing test, I'd expect us to have a test for the LSDA of a try-in-catch:

try { }
catch () {
  try { }
  catch () { }
}

As I understand it, the outer TBME must appear before the inner one, and the current behavior puts them in the other way around, innermost first.

In D77920#1984956, @rnk wrote:

Sounds good, and the fix looks simple, but please add a test.

I'm surprised this change doesn't break an existing test, I'd expect us to have a test for the LSDA of a try-in-catch:

try { }
catch () {
  try { }
  catch () { }
}

As I understand it, the outer TBME must appear before the inner one, and the current behavior puts them in the other way around, innermost first.

Are you questioning the throw in outer _try or inner _try? Windows runtime expects outer-first and inner-next ordering. It searches TBME table in REVERSED ordering. So for throw in inner _try, the inner catch is located. For the throw in outer _try, the State of it is lower then inner one. So the inner catch will be skipped and outer catch will be located. Hope this answer your question.

rnk added a comment.Apr 15 2020, 5:01 PM

Are you questioning the throw in outer _try or inner _try? Windows runtime expects outer-first and inner-next ordering. It searches TBME table in REVERSED ordering. So for throw in inner _try, the inner catch is located. For the throw in outer _try, the State of it is lower then inner one. So the inner catch will be skipped and outer catch will be located. Hope this answer your question.

Yep, this makes sense, I understand how we want to rearrange the table.

My concern is that I thought we already have in-tree tests for this case, and I can see from the Harbormaster link on this patch that they will fail:
https://reviews.llvm.org/harbormaster/unit/view/57903/
LLVM.CodeGen/X86::win-catchpad-nested-cxx.ll

So, those test expectations need to be updated to land this patch.

right, thank you Reid. I see it now :

; CHECK: $tryMap$try_in_catch:
; CHECK-NEXT: .long 2
; CHECK-NEXT: .long 2
; CHECK-NEXT: .long 3
; CHECK-NEXT: .long 1
; CHECK-NEXT: .long ($handlerMap$0$try_in_catch)
; CHECK-NEXT: .long 0
; CHECK-NEXT: .long 0
; CHECK-NEXT: .long 3
; CHECK-NEXT: .long 1
; CHECK-NEXT: .long ($handlerMap$1$try_in_catch)

the order must be reversed.

aganea added a subscriber: aganea.Apr 23 2020, 5:53 PM

Thanks for all the comments! Does this look good? Or other considerations?

LGTM. Anyone else have comments?

rnk accepted this revision.May 19 2020, 11:33 AM

The code change looks correct, but the test still needs to be updated. I'll go ahead and mark it accepted, but please remember to do that when landing this.

This revision is now accepted and ready to land.May 19 2020, 11:33 AM