Page MenuHomePhabricator

[X86] When using Win64 ABI, exit with error if SSE is disabled for varargs
ClosedPublic

Authored by aemerson on Jan 24 2018, 4:10 PM.

Details

Summary

We need XMM registers to handle varargs with the Win64 ABI. Before we would silently generate bad code resulting in an assertion failure elsewhere in the backend.

@rnk @thegameg I see there was similar work in D27522 for register returns. I don't know if report_fatal_error is ok to do here, AFAICT there's no way to do something sensible but die in this situation.

Diff Detail

Repository
rL LLVM

Event Timeline

aemerson created this revision.Jan 24 2018, 4:10 PM
thegameg added inline comments.Jan 24 2018, 5:12 PM
lib/Target/X86/X86ISelLowering.cpp
3565

What @rnk ended up doing in r302835 is emit a diagnostic (you can probably re-use errorUnsupported). The inconvenient with a diagnostic is that you might hit some asserts and you might have to set up some dummy state to satisfy the rest of the function. The advantage is that, if this is reached through a clang invocation the diagnostic will show up as a nice error while report_fatal_error will show up as a crash instructing the user to file a bug report.

Right, it's not clear to me how we'd avoid crashing later on since in this case it happens in a later pass, not in isel. If report_fatal_error does indeed always triggers crash diagnostics in clang then this is no better than assertion failures. Perhaps emitting the diagnostic first with errorUnsupported might be better than nothing.

rnk added a comment.Feb 7 2018, 10:55 AM

I wonder if it would be more general to handle this stuff earlier in the generated calling convention code, so that the use of XMM registers is conditionalized on the subtarget support. In other words, if SSE isn't available, we'd end up assigning FP values to stack locations, just as if all SSE registers had already been consumed. We could diagnose the error at that point. I think all this code is table-generated, which makes it hard to see and edit with these fixes.

RKSimon resigned from this revision.Feb 11 2018, 7:45 AM
aemerson updated this revision to Diff 135907.Feb 26 2018, 8:06 AM
aemerson removed a reviewer: RKSimon.

Changed to use errorUnsupported() so we get some diagnostics first.

@rnk I'm not exactly sure which calling convention code you're talking about. In any case, is this change reasonable?

rnk added a comment.Mar 2 2018, 1:59 PM

This doesn't fix the crash, though. We just assert later now.

In D42512#1026016, @rnk wrote:

This doesn't fix the crash, though. We just assert later now.

Correct, but we can at least get useful source diagnostics of the issue before it does. I also didn't understand what you meant by the your previous comment.

aemerson accepted this revision.Apr 2 2019, 3:45 PM

Improved the error message with r357317

This revision is now accepted and ready to land.Apr 2 2019, 3:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2019, 3:45 PM
aemerson closed this revision.Apr 2 2019, 3:45 PM

This patch is not correct -- the crash is not with varargs functions specifically, llvm also crashes for a function declared to explicitly take a float/double. The Win64 calling convention code assigns values to xmm registers, but it should not when sse is disabled.

Additionally, this patch unnecessarily breaks calling vararg functions with _integer_ arguments, which in particular, the EDK2 project does, with windows ABI functions and sse disabled. So, since this breaks existing working code, I'm going to revert this change now, and then propose a different change to address the crash.

This patch is not correct -- the crash is not with varargs functions specifically, llvm also crashes for a function declared to explicitly take a float/double. The Win64 calling convention code assigns values to xmm registers, but it should not when sse is disabled.

Additionally, this patch unnecessarily breaks calling vararg functions with _integer_ arguments, which in particular, the EDK2 project does, with windows ABI functions and sse disabled. So, since this breaks existing working code, I'm going to revert this change now, and then propose a different change to address the crash.

Hi James,

Do you have a better fix for this in that case?

Thanks,
Amara