Register clobber information for asm blocks jumped to can be incorrect

[ehsan]

Bugzilla Link	21035
Version	trunk
OS	All
CC	@majnemer,@zmodem,@jrmuizel,@rnk

Extended Description

Consider this test case:

void f() { asm { mov edx, 1 jmp lbl } asm { lbl: mov ecx, 2 } }

We currently generate the following code:

define void @​f() #​0 { entry: tail call void asm sideeffect inteldialect "mov edx, $$1\0A\09jmp LMSASMLABEL.0lbl", "~{edx},~{dirflag},~{fpsr},~{flags}"() #​1, !srcloc !​5 tail call void asm sideeffect inteldialect "LMSASMLABEL.0lbl:\0A\09mov ecx, $$2", "~{ecx},~{dirflag},~{fpsr},~{flags}"() #​1, !srcloc !​6 ret void }

However, the second asm block also clobbers edx, through the jump from the first one.

[ehsan]

New fix: http://reviews.llvm.org/D5694

[ehsan]

These patches add a diagnostic for these kinds of jumps:

http://reviews.llvm.org/D5515 http://reviews.llvm.org/D5516

[ehsan]

Branch instructions from inside an asm block to anywhere that isn't inside that same asm block (or to the start of another function, e.g., tail calls) are going to cause major problems. Callee saved registers are the tip of the iceberg. Those are edges of the CFG that the compiler doesn't know about which will completely break SSA.

You're right...

I'm working on a patch to diagnose if we attempt to jump from one asm block to another. Still not sure if my approach is going to work or not. I'm extending AsmParser::parseMSInlineAsm to return a list of "branch targets" for each asm block. Each branch target is the internal name for the label if we detect a branch instruction in the MC layer. Once this information is exposed, I hope to be able to extend JumpDiagnostics.cpp to detect when there is a branch target with the same internal name as that of a LabelDecl created from an asm block, and diagnose that.

[llvmbot]

Branch instructions from inside an asm block to anywhere that isn't inside that same asm block (or to the start of another function, e.g., tail calls) are going to cause major problems. Callee saved registers are the tip of the iceberg. Those are edges of the CFG that the compiler doesn't know about which will completely break SSA.

[rnk]

Do you know who might know how to do this?

Woops, I didn't answer this. Try Jim Grosbach on IRC maybe? I think he's just 'grosbach'.

I believe TargetRegisterInfo is the consumer of MCRegisterInfo.

Well, sure. But it's in Target, and I think MC is supposed to not depend on Target (otherwise the current setup of things is very weird!). Also, more importantly, MC should not be architecture dependent, right? I think what we need here is a cross target way of getting the list of target specific general purpose registers.

Pretty much. Going beyond GPRs we might want to mark XMM registers as clobbered, but that can come later.

[ehsan]

Personally, I think this is the right way to go. If we see either labels or control flow, we should consider all registers other than esp and ebp to be clobbered.

In theory, inline assembly can clobber esp and ebp too, right?

MSVC warns you if you clobber ebp, so if you do that, you're on your own.

Interesting. I'll look into adding a warning for that too.

You can no longer refer to locals. It is possible to adjust esp as you execute, in which case we should address locals via ebp. I suppose you could add esp to the clobber set of code with control flow.

OK, I will look into this later.

We also have to worry about the base pointer if stack realignment is required, but that can be a separate issue.

I have a vague memory of seeing some code that dealt with stack realignment for inline asm blocks, but I can't find it right now... But yeah, we can address that separately.

I looked at MCRegisterInfo, and I'm not sure how to find all the GPRs. =/ There's got to be some way to ask for the set of allocatable registers.

Looking at X86GenRegisterInfo.inc as an example, it seems like we should look at regclasses. For X86, we initialize the Classes field to X86MCRegisterClasses, which contains "GR32" which is I think what we want but iterating over MCRegisterInfo classes and looking for "GR32" sounds very hacky. To make things more complicated, I cannot find any consumer for the register class concept in the code base (there is some for the similar TargetRegisterInfo type.)

Do you know who might know how to do this?

I believe TargetRegisterInfo is the consumer of MCRegisterInfo.

Well, sure. But it's in Target, and I think MC is supposed to not depend on Target (otherwise the current setup of things is very weird!). Also, more importantly, MC should not be architecture dependent, right? I think what we need here is a cross target way of getting the list of target specific general purpose registers.

[rnk]

Personally, I think this is the right way to go. If we see either labels or control flow, we should consider all registers other than esp and ebp to be clobbered.

In theory, inline assembly can clobber esp and ebp too, right?

MSVC warns you if you clobber ebp, so if you do that, you're on your own. You can no longer refer to locals. It is possible to adjust esp as you execute, in which case we should address locals via ebp. I suppose you could add esp to the clobber set of code with control flow.

We also have to worry about the base pointer if stack realignment is required, but that can be a separate issue.

I have a vague memory of seeing some code that dealt with stack realignment for inline asm blocks, but I can't find it right now... But yeah, we can address that separately.

I looked at MCRegisterInfo, and I'm not sure how to find all the GPRs. =/ There's got to be some way to ask for the set of allocatable registers.

Looking at X86GenRegisterInfo.inc as an example, it seems like we should look at regclasses. For X86, we initialize the Classes field to X86MCRegisterClasses, which contains "GR32" which is I think what we want but iterating over MCRegisterInfo classes and looking for "GR32" sounds very hacky. To make things more complicated, I cannot find any consumer for the register class concept in the code base (there is some for the similar TargetRegisterInfo type.)

Do you know who might know how to do this?

I believe TargetRegisterInfo is the consumer of MCRegisterInfo.

[ehsan]

Personally, I think this is the right way to go. If we see either labels or control flow, we should consider all registers other than esp and ebp to be clobbered.

In theory, inline assembly can clobber esp and ebp too, right?

We also have to worry about the base pointer if stack realignment is required, but that can be a separate issue.

I have a vague memory of seeing some code that dealt with stack realignment for inline asm blocks, but I can't find it right now... But yeah, we can address that separately.

I looked at MCRegisterInfo, and I'm not sure how to find all the GPRs. =/ There's got to be some way to ask for the set of allocatable registers.

Looking at X86GenRegisterInfo.inc as an example, it seems like we should look at regclasses. For X86, we initialize the Classes field to X86MCRegisterClasses, which contains "GR32" which is I think what we want but iterating over MCRegisterInfo classes and looking for "GR32" sounds very hacky. To make things more complicated, I cannot find any consumer for the register class concept in the code base (there is some for the similar TargetRegisterInfo type.)

Do you know who might know how to do this?

[rnk]

Personally, I think this is the right way to go. If we see either labels or control flow, we should consider all registers other than esp and ebp to be clobbered.

We also have to worry about the base pointer if stack realignment is required, but that can be a separate issue.

I looked at MCRegisterInfo, and I'm not sure how to find all the GPRs. =/ There's got to be some way to ask for the set of allocatable registers.

[ehsan]

The MC asm parser code sees each block individually, so I don't think we can solve this very easily. But one thing that should at least help us generate correct if not optimal code is to consider a label inside an asm block as a hint to clobber all registers.

I tried to write a fix for it https://gist.github.com/ehsan/d45e2ca33a922feb6c89, but I got stuck trying to figure out how to get a list of all registers. It seems like iterating over the [0,getNumRegs) range is not the right way. Any ideas?

[ehsan]

assigned to @ehsan