Page MenuHomePhabricator

[AArch64][Windows] Compute function length correctly in unwind tables.
ClosedPublic

Authored by efriedma on Apr 24 2019, 2:57 PM.

Details

Summary

The primary fix here is to WinException.cpp: we need to exclude jump tables when computing the length of a function, or else we fail to correctly compute the length. (We can only compute the number of bytes consumed by certain assembler directives after the entire file is parsed. ".p2align" is one of those directives, and is used by jump table generation.)

The secondary fix, to MCWin64EH, is to make sure we don't silently miscompile if we hit a similar situation in the future.

It's possible we could extend ARM64EmitUnwindInfo to emit the length of the function using a relocation that would be resolved later in the assembler, instead of trying to resolve it immediately; this would make alignment directives work correctly. I haven't pursued that approach because I'm not sure how complicated it would be. (We would need to estimate the length somehow to implement unwind data splitting).

Hopefully fixes https://bugs.llvm.org/show_bug.cgi?id=41581 .

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Apr 24 2019, 2:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2019, 2:57 PM
rnk added a comment.Apr 24 2019, 3:21 PM

It sounds like we're encoding the unwind info too early for AArch64. When we were implementing codeview inline line table stuff, we needed to be able to reliably calculate absolute label differences, so we added MCCVInlineLineTableFragment to delay the table encoding until assembly time. We can certainly do this for now, but perhaps a FIXME to delay things would be good.

Well, I guess there are sort of two potential approaches to emitting it "later". One, we could emit something like ".word (endfunc-beginfunc)/4+0x10800000", as long as we have some conservative estimate we could use to prove that we don't need to split the unwind data. The other would be to use a dedicated, relaxable fragment, which could grow to accommodate splitting the unwind data if necessary. The dedicated fragment is probably a bit more bulletproof, and more consistent with the way we handle relaxation in other contexts, but it would require a lot of refactoring to implement.

I can put a FIXME into ARM64EmitUnwindInfo with the above paragraph, sure. (I don't think the WinException changes need a FIXME; even if we do fix the assembler side of this issue, there isn't any reason for the unwind table to provide unwind information for jump tables.)

rnk added a comment.Apr 24 2019, 4:43 PM

Well, I guess there are sort of two potential approaches to emitting it "later". One, we could emit something like ".word (endfunc-beginfunc)/4+0x10800000", as long as we have some conservative estimate we could use to prove that we don't need to split the unwind data. The other would be to use a dedicated, relaxable fragment, which could grow to accommodate splitting the unwind data if necessary. The dedicated fragment is probably a bit more bulletproof, and more consistent with the way we handle relaxation in other contexts, but it would require a lot of refactoring to implement.

Oh, yes, that would be the usual way of doing things, build a symbolic MCExpr for it. With the inline line table encoding, we couldn't do that because the label differences get encoded with a variable length encoding. Anyway, a FIXME is fine.

I've confirmed that this fixes the Firefox repro, thanks!

@efriedma - what's the status on this one?

I've been distracted by some other work; I'll try to address the review comments today or tomorrow.

efriedma updated this revision to Diff 197492.Apr 30 2019, 5:53 PM

Added FIXME to MCWin64EH.cpp.

rnk accepted this revision.May 1 2019, 1:05 PM

lgtm, thanks!

This revision is now accepted and ready to land.May 1 2019, 1:05 PM
This revision was automatically updated to reflect the committed changes.