Page MenuHomePhabricator

[X86] Fix crash with inline asm using wrong register name
ClosedPublic

Authored by fwolff on Sat, Nov 13, 1:24 PM.

Details

Summary

Fixes PR#48678. X86TargetLowering::getRegForInlineAsmConstraint() can adjust the register class to match the type, e.g. change VR128X to VR256X if the type needs 256 bits. However, the function currently returns the unadjusted register and the adjusted register class, e.g. xmm15 and VR256X, which then causes an assertion failure later because the register class does not contain that register. This patch fixes this behavior.

Diff Detail

Event Timeline

fwolff created this revision.Sat, Nov 13, 1:24 PM
fwolff requested review of this revision.Sat, Nov 13, 1:24 PM

I'm not sure if this is the right fix. The input somehow looks undefined behavior, which means we don't necessarily follow what's GCC doing. Besides, GCC doesn't handle it well in other case https://godbolt.org/z/6TTEvKvT6
Can we check for this case and simply report error/warning in the front end?

llvm/lib/Target/X86/X86ISelLowering.cpp
54134 ↗(On Diff #387046)

Should be better to pass Res directly?

fwolff updated this revision to Diff 387110.Sun, Nov 14, 10:52 AM

I've changed it to issue an error instead now. Is this what you had in mind @pengfei?

RKSimon added inline comments.Sun, Nov 14, 2:02 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
8408

return None ?

fwolff updated this revision to Diff 387142.Sun, Nov 14, 3:48 PM

Use return None; instead of return {};.

fwolff marked an inline comment as done.Sun, Nov 14, 3:49 PM
pengfei accepted this revision.Sun, Nov 14, 10:51 PM

LGTM.

This revision is now accepted and ready to land.Sun, Nov 14, 10:51 PM

Thanks again for the review. Can you also commit this for me @pengfei? (The libFuzzer test failure seems unrelated.)

This revision was landed with ongoing or failed builds.Mon, Nov 15, 6:38 PM
This revision was automatically updated to reflect the committed changes.