Differential D42512
[X86] When using Win64 ABI, exit with error if SSE is disabled for varargs Authored by aemerson on Jan 24 2018, 4:10 PM.
Details 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
Event Timeline
Comment Actions 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. Comment Actions 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. Comment Actions 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? Comment Actions 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. Comment Actions 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. | ||||||
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.