LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 1398 - wrong code (bad esp update) with -enable-llvm on x86
Summary: wrong code (bad esp update) with -enable-llvm on x86
Status: RESOLVED FIXED
Alias: None
Product: new-bugs
Classification: Unclassified
Component: new bugs (show other bugs)
Version: unspecified
Hardware: Other Linux
: P normal
Assignee: Unassigned LLVM Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-05-07 11:02 PDT by Duncan Sands
Modified: 2010-02-22 12:48 PST (History)
3 users (show)

See Also:
Fixed By Commit(s):


Attachments
testcase .ll (52.85 KB, text/plain)
2007-05-07 11:03 PDT, Duncan Sands
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Duncan Sands 2007-05-07 11:02:00 PDT
I've been seeing mysterious segfaults when compiling using
-enable-llvm, even though no exceptions are raised.  It turns
out to be due to a wrong stack pointer update:

$ llc ol.bc -o no_eh
$ llc ol.bc -enable-eh -o eh 
$ diff no_eh eh
...
144c153,155
<       addl $12, %esp
---
>       addl $16, %esp

If I change the $16 to $12 in the eh .s, then
the segfault goes away and all is well.
Comment 1 Duncan Sands 2007-05-07 11:03:14 PDT
Created attachment 834 [details]
testcase .ll
Comment 2 Anton Korobeynikov 2007-05-07 11:11:34 PDT
Does code generated with -disable-fp-elim fail too?
Comment 3 Duncan Sands 2007-05-07 11:40:30 PDT
Yes, it still fails with -disable-fp-elim.  In fact
-disable-fp-elim doesn't result in any changes for the
first 500 lines of assembler or so.  With or without
-disable-fp-elim, the frame pointer (ebp) is still being
set up at the start of the function and used in a bunch
of places (not everywhere)!
Comment 4 Duncan Sands 2007-05-07 12:03:54 PDT
The reason -disable-fp-elim has no particular effect is that the function
contains variable sized allocas:
// hasFP - Return true if the specified function should have a dedicated frame
// pointer register.  This is true if the function has variable sized allocas 
or
// if frame pointer elimination is disabled.

Comment 5 Duncan Sands 2007-05-07 12:12:13 PDT
From X86RegisterInfo.cpp:956:

        assert(Old->getOpcode() == X86::ADJCALLSTACKUP);
        // factor out the amount the callee already popped.
        uint64_t CalleeAmt = Old->getOperand(1).getImm();
        Amount -= CalleeAmt;

With -enable-eh, CalleeAmt is 0; without it CalleeAmt is 4.
Presumably this is caused somehow by a difference between
invoke and call.
Comment 6 Anton Korobeynikov 2007-05-07 12:19:13 PDT
Hrm, this is pretty strange... I'll have a look.
Comment 7 Duncan Sands 2007-05-07 12:23:24 PDT
It may come from here:

  // If the first argument is an sret pointer, remember it.
  bool isSRet = NumOps &&
    (cast<ConstantSDNode>(Op.getOperand(6))->getValue() &
     ISD::ParamFlags::StructReturn);

Whether isSRet is true or not results in a 4 byte difference
in the stack popping after a call.  It is true the first two
times I get here without -enable-eh, and true then false with
-enable-eh.  Of course it is possible that these don't correspond
to the same calls, if the DAG is being visited in a different
order...
Comment 8 Duncan Sands 2007-05-07 12:28:41 PDT
The invoke which gets the wrong stack pop amount after it, is

invoke void @ol__normalize_pathname__get_directory.180
( %struct.string___XUP* %tmp59 
sret , %struct.FRAME.ol__normalize_pathname* %FRAME.11, i64 %tmp255 )

which is indeed a struct return.
Comment 9 Anton Korobeynikov 2007-05-07 12:51:53 PDT
Well, if case of struct return callee should pop hidden pointer from stack. So,
12 is right and 16 is not.
Comment 10 Duncan Sands 2007-05-07 13:21:52 PDT
> Well, if case of struct return callee should pop hidden pointer from stack.
> So, 12 is right and 16 is not.

Exactly: with 12 it works, with 16 it dies horribly.
Comment 11 Anton Korobeynikov 2007-05-07 13:50:13 PDT
Investigating
Comment 12 Duncan Sands 2007-05-07 14:47:06 PDT
Solution proposed by aKor on IRC:

(09:32:17 PM) aKor[off-site]: in SelectionDAGLowering::LowerCallTo
(09:32:49 PM) aKor[off-site]: change paramHasAttr(i, ...) to 
paramHasAttr(i-OpIdx+1, ...)
(09:33:00 PM) aKor[off-site]: i is operand index
(09:33:03 PM) aKor[off-site]: not parameter
(09:33:09 PM) aKor[off-site]: it's ok for calls
(09:33:11 PM) aKor[off-site]: but not for invokes