Re: compiler bug turning up in cmake package?

Re: compiler bug turning up in cmake package?

[Andrew Haley]

Matthew Woehlke wrote:
> I'm getting this SEGV trying to install kdelibs on my machine (koji
> package 2.6.1-1.fc10.i386):
> 
> #0  cmELF::GetRPath (this=0xbf986708) at
> /usr/src/debug/cmake-2.6.1/Source/cmELF.cxx:787
> #1  0x080e9c07 in cmSystemTools::CheckRPath (file=@0xbf986800,
> newRPath=@0xbf9867fc)
>     at /usr/src/debug/cmake-2.6.1/Source/cmSystemTools.cxx:2617
> #2  0x08134378 in cmFileCommand::HandleRPathCheckCommand
> (this=0x9ad3db8, args=@0xbf986874)
>     at /usr/src/debug/cmake-2.6.1/Source/cmFileCommand.cxx:1557
> #3  0x0815dd6a in cmFileCommand::InitialPass (this=0x9ad3db8,
> args=@0xbf986874)
>     at /usr/src/debug/cmake-2.6.1/Source/cmFileCommand.cxx:121
> #4  0x081620cc in cmCommand::InvokeInitialPass (this=0x9ad3db8,
> args=@0x9ad3fd4, status=@0xbf986918)
>     at /usr/src/debug/cmake-2.6.1/Source/cmCommand.h:68
> #5  0x080c65a8 in cmMakefile::ExecuteCommand (this=0x9ac2730,
> lff=@0x9ad3fc8, status=@0xbf986918)
>     at /usr/src/debug/cmake-2.6.1/Source/cmMakefile.cxx:399
> #6  0x08150baa in cmIfFunctionBlocker::IsFunctionBlocked
> (this=0x9ad1170, lff=@0x9ad7f98, mf=@0x9ac2730,
>     inStatus=@0xbf9869f8) at
> /usr/src/debug/cmake-2.6.1/Source/cmIfCommand.cxx:116
> #7  0x080b95dc in cmMakefile::IsFunctionBlocked (this=0x9ac2730,
> lff=@0x9ad7f98, status=@0xbf9869f8)
>     at /usr/src/debug/cmake-2.6.1/Source/cmMakefile.cxx:2303
> 
> The relevant code is pretty boring:
> 
> 785     bool cmELF::Valid() const
> 786     {
> 787       return this->Internal && this->Internal->GetFileType() !=
> FileTypeInvalid;
> 788     }
> 
> ...but the disassembly is unnerving:
> 
> 0x819d4c0 <_ZN5cmELF8GetRPathEv>:       push   %ebp
> 0x819d4c1 <_ZN5cmELF8GetRPathEv+1>:     mov    %esp,%ebp

I think you may be right.

First passed arg is at %ebp + 8.

The stack is:

  this
  retaddr
  prev frame   <--  %ebp

> 0x819d4c3 <_ZN5cmELF8GetRPathEv+3>:     sub    $0x8,%esp
> 0x819d4c6 <_ZN5cmELF8GetRPathEv+6>:     mov    0x8(%ebp),%eax

%eax now contains this

> 0x819d4c9 <_ZN5cmELF8GetRPathEv+9>:     mov    (%eax),%edx

First word of object -> %edx

> 0x819d4cb <_ZN5cmELF8GetRPathEv+11>:    test   %edx,%edx
> 0x819d4cd <_ZN5cmELF8GetRPathEv+13>:    je     0x819d510 <_ZN5cmELF8GetRPathEv+80>
> 0x819d4cf <_ZN5cmELF8GetRPathEv+15>:    mov    0x10(%edx),%eax
> 0x819d4d2 <_ZN5cmELF8GetRPathEv+18>:    test   %eax,%eax
> 0x819d4d4 <_ZN5cmELF8GetRPathEv+20>:    je     0x819d500 <_ZN5cmELF8GetRPathEv+64>
> 0x819d4d6 <_ZN5cmELF8GetRPathEv+22>:    cmp    $0x2,%eax
> 0x819d4d9 <_ZN5cmELF8GetRPathEv+25>:    je     0x819d4e2 <_ZN5cmELF8GetRPathEv+34>
> 0x819d4db <_ZN5cmELF8GetRPathEv+27>:    cmp    $0x3,%eax
> 0x819d4de <_ZN5cmELF8GetRPathEv+30>:    xchg   %ax,%ax
> 0x819d4e0 <_ZN5cmELF8GetRPathEv+32>:    jne    0x819d500 <_ZN5cmELF8GetRPathEv+64>
> 0x819d4e2 <_ZN5cmELF8GetRPathEv+34>:    mov    (%edx),%eax
> 0x819d4e4 <_ZN5cmELF8GetRPathEv+36>:    movl   $0xf,0x4(%esp)
> 0x819d4ec <_ZN5cmELF8GetRPathEv+44>:    mov    %edx,(%esp)
> 0x819d4ef <_ZN5cmELF8GetRPathEv+47>:    call   *0x14(%eax)
> 0x819d4f2 <_ZN5cmELF8GetRPathEv+50>:    leave
> 0x819d4f3 <_ZN5cmELF8GetRPathEv+51>:    nop
> 0x819d4f4 <_ZN5cmELF8GetRPathEv+52>:    lea    0x0(%esi,%eiz,1),%esi
> 0x819d4f8 <_ZN5cmELF8GetRPathEv+56>:    ret
> 0x819d4f9 <_ZN5cmELF8GetRPathEv+57>:    lea    0x0(%esi,%eiz,1),%esi
> 0x819d500 <_ZN5cmELF8GetRPathEv+64>:    xor    %eax,%eax
> 0x819d502 <_ZN5cmELF8GetRPathEv+66>:    leave
> 0x819d503 <_ZN5cmELF8GetRPathEv+67>:    nop
> 0x819d504 <_ZN5cmELF8GetRPathEv+68>:    lea    0x0(%esi,%eiz,1),%esi
> 0x819d508 <_ZN5cmELF8GetRPathEv+72>:    ret
> 0x819d509 <_ZN5cmELF8GetRPathEv+73>:    lea    0x0(%esi,%eiz,1),%esi
> 0x819d510 <_ZN5cmELF8GetRPathEv+80>:    mov    0x10(%edx),%eax
> 0x819d513 <_ZN5cmELF8GetRPathEv+83>:    nop
> 0x819d514 <_ZN5cmELF8GetRPathEv+84>:    lea    0x0(%esi,%eiz,1),%esi
> 0x819d518 <_ZN5cmELF8GetRPathEv+88>:    jmp    0x819d4db
> <_ZN5cmELF8GetRPathEv+27>
> 
> Look particularly at the test at +11 and jump at +13, and then at lines
> +80 and +15. If I read this right, it tests if "this->Internal" is NULL,
> and then *dereferences it either way*. This is clearly not what the
> source listing says (and is clearly wrong), so I wonder where this
> generated code came from.
> 
> Hmm, actually, staring it it, trying to figure out how to hot-hack it so
> the install will finish, it looks like the jump address is wrong (should
> be going to +64, not +80). Or else, something funny is happening w.r.t.
> "Internal"s vtable.

I think you need to press ahead at full speed to generate a standalone
test case from this.

Andrew.

Re: compiler bug turning up in cmake package?

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Matthew Woehlke wrote:
> The relevant code is pretty boring:
> 
> 785     bool cmELF::Valid() const
> 786     {
> 787       return this->Internal && this->Internal->GetFileType() !=
> FileTypeInvalid;
> 788     }

It's most certainly not that simple.  I haven't looked at the sources.
But the asm code does not really correspond to the code above.  The
function above is most certainly inlined.  The problem might very well
(and most likely is) in the use of this function.  Look at the

  cmELF::GetRPath()

function and where it directly or indirectly uses the Valid function.

- --
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iEYEARECAAYFAki0FqoACgkQ2ijCOnn/RHSU1gCfTrXyNODqmhe8HVq0/TqUimNP
qp8AoLLh96ff+ujzn7Foq7nULxhvivtX
=br4V
-----END PGP SIGNATURE-----

Re: compiler bug turning up in cmake package?

[John Fine]

Ulrich Drepper wrote:
> It's most certainly not that simple. I haven't looked at the sources.
> But the asm code does not really correspond to the code above.  The
> function above is most certainly inlined.  The problem might very well
> (and most likely is) in the use of this function.  Look at the
>
>   cmELF::GetRPath()
>
> function and where it directly or indirectly uses the Valid function.
>
>   
It is very simple and not a compiler bug, and you are correct that the 
error is in GetRPath.

The code is

  if(this->Valid() &&
     this->Internal->GetFileType() == cmELF::FileTypeExecutable ||
     this->Internal->GetFileType() == cmELF::FileTypeSharedLibrary)

Notice the lack of () around the || lines

So that if Valid returns false it then tries
this->Internal->GetFileType() ==
and crashes.

Re: compiler bug turning up in cmake package?

[Matthew Woehlke]

John Fine wrote:
> Ulrich Drepper wrote:
>> It's most certainly not that simple. I haven't looked at the sources.
>> But the asm code does not really correspond to the code above.  The
>> function above is most certainly inlined.  The problem might very well
>> (and most likely is) in the use of this function.  Look at the
>>
>>   cmELF::GetRPath()
>>
>> function and where it directly or indirectly uses the Valid function.
>   
> It is very simple and not a compiler bug, and you are correct that the 
> error is in GetRPath.
> 
> The code is
> 
>  if(this->Valid() &&
>     this->Internal->GetFileType() == cmELF::FileTypeExecutable ||
>     this->Internal->GetFileType() == cmELF::FileTypeSharedLibrary)
> 
> Notice the lack of () around the || lines
> 
> So that if Valid returns false it then tries
> this->Internal->GetFileType() ==
> and crashes.

Oops! Ok, that looks consistent with the disassembly, thanks for the 
analysis! Hehe, someone isn't listening to the 'suggest parentheses' 
warning I bet ;-). CC'ing gcc-help one last time so those folk know it's 
resolved, but I imagine we can drop them from any future correspondence.

Obviously this should be fixed upstream, but will we be able to do a 
patched fedora build before this goes into F10-stable? (And, er, 
incidentally, so I can install a fixed rpm? Guess I have good timing 
catching it now, eh? ;-) )

-- 
Matthew
Person A: It's an ISO standard.
Person B: ...And that means what?
   --mal (http://theangryadmin.blogspot.com/2008/04/future.html)

Re: compiler bug turning up in cmake package?

[John Fine]

Matthew Woehlke wrote:
> . CC'ing gcc-help one last time so those folk know it's resolved, but 
> I imagine we can drop them from any future correspondence.
>
Maybe gcc-help should get a bit more of a comment on the small "almost a 
bug" that caused the fault on line 888 to be misreported as line 787.

In general in optimized code there is no way to correctly match machine 
instructions to source lines.  But in this particular case, a bit more 
book keeping in GCC could have matched it.

The optimizer has almost fully merged the this->Internal->GetFileType() 
calls on lines 787, 887 and 888.

The machine instruction at _ZN5cmELF8GetRPathEv+15 is actually all three 
of those and thus cannot be correctly tied to its source line.

But the machine instruction at _ZN5cmELF8GetRPathEv+80 (the one that 
faulted) represents only line 888 (and in fact only when 888 is reached 
without executing 887).  That instruction could be attributed correctly 
to source line 888 rather than incorrectly to 787.

I assume (without digging into the relevant gcc source code) that in the 
process of merging the this->Internal->GetFileType() on line 888 with 
the one on 787 and then unmerging the two paths into line 888, gcc has 
lost track of the fact that one path into 888 is not actually merged 
with 787.

While it is impossible to consistently attribute such faults to the 
correct source line, it is always nicer to correctly attribute the ones 
that could be correctly attributed.

Also, where the optimizer has done enough data flow analysis (as it must 
have to rearrange this way) to know for certain that a particular 
machine instruction will always be dereferencing null, maybe there ought 
to be some warning or other special behavior.  (Line 888 doesn't always 
dereference null, but once the two paths to 888 have been split, there 
is machine code generated that the optimizer should know must fault).