[llvm-commits] [llvm-gcc-4.2] r136345 - /llvm-gcc-4.2/trunk/gcc/llvm-convert.cpp

[Duncan Sands]

Hi Bob, after thinking about this some more, I think it is reasonable to add
support for a new global variable initializer having a different type to the
original (i.e. directly addressing the ObjC problem).  Right now, places that
directly convert a T* will get a convert(T)*; it's only a T* that occurs inside
a struct that might get converted oddly.  Code like the COMPONENT_REF logic
that dig down into structs do the right thing because they only convert
pointers directly, and don't assume that struct fields of pointer type have the
right type.  So I don't think anything can go wrong.

I will try to do something within the next few days.

Ciao, Duncan.

>>>>> commit 136600 hopefully fixed these failures.
>>>>
>>>> I haven't confirmed yet whether it fixed the ARM problems (due to other issues),
>>>> but it seems to be causing massive failures on i386:
>>>>
>>>> http://llvm.org/perf/db_default/simple/nts/263/
>>>>
>>>> I can reproduce some of those failures with -O0 and we're not seeing any issues
>>>> with clang, so I think it's due to your change. I'm going to revert it for now
>>>> and re-run the tests to confirm that.
>>>
>>> OK, sorry for the breakage.  I only tested on x86-64, I will take a look at
>>> i386.
>>
>> Duncan, are you still planning to get to this?  Those ObjC unit tests are still failing.
>
> not really.  The basic problem is that the same GCC pointer type is
> converted into more than one LLVM type, depending on whether it is
> converted as part of converting an enclosing struct or not.  In the
> ObjC testcases this results in an apparent change in the initializer:
> the initializer is basically unchanged, only the type of some pointers
> changed.
>
> This can be fixed by caching the result of the pointer conversion the
> first time it happens, and then using that everywhere.  This resulted
> in miscompiles on x86 (and presumably elsewhere, only less spectacular),
> because code in various parts of llvm-gcc assumes that if you convert
> a T*, then you get a pointer to convert(T).  For example, to evaluate
> p + n where p is a T*, it (IIRC) just does a "GEP p, n".  But if sometimes
> T* is converted to i8*, then "GEP p, n" advances by n bytes rather than
> by n * sizeof(T) bytes.
>
> In fact, if you think about it, if ever convert(T*) != convert(T)*, then
> there is a chance of things going wrong.  Unfortunately the new type
> system requires that occasionally T* not be converted to convert(T)*.
>
> The right thing to do is to audit all of the llvm-gcc code for places
> where it makes assumptions about how T* is converted, and fix them all.
> I actually did this in dragonegg some time ago, which is why it doesn't
> suffer like llvm-gcc does from the new type system.
>
> The quick and nasty thing to do is to paper over problems until you don't
> notice problems any more.  For example, fix the objc testcases by handling the
> case where the new initializer doesn't have the same type as the old one.
>
> I'm not interested in auditing llvm-gcc's code (though probably there wouldn't
> be too much to check).  I'm also not interested in hacking in a nasty "fix".
> Thus I'm not planning to do anything about this.
>
> Ciao, Duncan.
>
> PS: You could revert the change that fixed the Fortran builders, and stop
> those builders from building Fortran.  That would also just be papering over
> the underlying problem, but maybe is acceptable.