Page MenuHomePhabricator

[X86] Print register names in .seh_* directives
ClosedPublic

Authored by rnk on Aug 22 2019, 4:31 PM.

Details

Summary

Also improve assembler parser register validation for .seh_ directives.
This requires moving X86-specific seh directive handling into the x86
backend, which addresses some assembler FIXMEs.

Diff Detail

Event Timeline

rnk created this revision.Aug 22 2019, 4:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2019, 4:31 PM
pengfei added inline comments.Aug 22 2019, 6:42 PM
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
3764

I saw lots of mixed use of register name and integer number in test cases. I think it may confuse user. Shall we disable the integer number supporting, or give a warning?

llvm/test/MC/AsmParser/directive_seh.s
22

Seems duplicated

rnk marked 2 inline comments as done.Aug 23 2019, 11:09 AM
rnk added inline comments.
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
3764

I think we should continue to accept the numbers to be GNU as compatible. I think the directive_seh.s and seh-directive-errors.s tests are the only remaining tests that use the numbers now, and they only exist to ensure we do the right thing with them.

llvm/test/MC/AsmParser/directive_seh.s
22

Oops, this was supposed to use a the number, but my sed command turned it into a register.

LGTM. But I'd like others to make a decision. Thanks!

rnk added a comment.Aug 30 2019, 1:50 PM

LGTM. But I'd like others to make a decision. Thanks!

Thanks! I was hoping to hear from @craig.topper or @RKSimon, but I've been meaning to do this for so long, I think it's time to move forward on it.

craig.topper added inline comments.Aug 30 2019, 2:18 PM
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
3791

Do we need to support 32-bit registers in 32-bit mode? Or is there soemthing about .seh that is only 64-bit?

This revision was not accepted when it landed; it landed in state Needs Review.Aug 30 2019, 2:25 PM
This revision was automatically updated to reflect the committed changes.
rnk marked an inline comment as done.Aug 30 2019, 3:23 PM
rnk added inline comments.
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
3791

These directives should be 64-bit only. There are tests for that at llvm/test/MC/AsmParser/seh-directive-errors.s. I noticed a crash, I'll fix that in a sec.

aganea added a subscriber: aganea.Sep 6 2019, 8:18 AM
aganea added inline comments.
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
3612

if (IDVal == ".seh_stackalloc") is not being handled here, but you've added X86AsmParser::parseDirectiveSEHAllocStack. Is that intended?

rnk marked an inline comment as done.Sep 6 2019, 11:06 AM
rnk added inline comments.
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
3612

No, that's a mistake, thanks. I was surprised to discover that the AArch64 SEH asm parsing code supports this directive and does something with it, so I had to move it back to MCStreamer, and forgot to delete this.