search

Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

Register clobber information for asm blocks jumped to can be incorrect #21034

Open Quuxplusone opened 10 years ago

Quuxplusone commented 10 years ago
Bugzilla Link PR21035
Status NEW
Importance P normal
Reported by Ehsan Akhgari [:ehsan] (ehsan.akhgari@gmail.com)
Reported on 2014-09-22 21:38:15 -0700
Last modified on 2014-10-08 22:11:45 -0700
Version trunk
Hardware PC All
CC david.majnemer@gmail.com, grosbach@apple.com, hans@chromium.org, jmuizelaar@mozilla.com, llvm-bugs@lists.llvm.org, rnk@google.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also 
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 L__MSASMLABEL_.0__lbl", "~{edx},~{dirflag},~{fpsr},~{flags}"() #1, !srcloc !5
  tail call void asm sideeffect inteldialect "L__MSASMLABEL_.0__lbl:\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.
Quuxplusone commented 10 years ago

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?

Quuxplusone commented 10 years ago

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.

Quuxplusone commented 10 years ago 
(In reply to comment #2)
> 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?
Quuxplusone commented 10 years ago 
(In reply to comment #3)
> (In reply to comment #2)
> > 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.
Quuxplusone commented 10 years ago 
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > 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.
Quuxplusone commented 10 years ago 
(In reply to comment #5)
> > > 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.
Quuxplusone commented 10 years ago

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.

Quuxplusone commented 10 years ago 
(In reply to comment #7)
> 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.
Quuxplusone commented 10 years ago 
These patches add a diagnostic for these kinds of jumps:

http://reviews.llvm.org/D5515
http://reviews.llvm.org/D5516
Quuxplusone commented 10 years ago

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