Fix miscompiled 32bit binaries by mingw
AbandonedPublic

Authored by yvvan on Jun 30 2017, 4:30 AM.

Details

Reviewers
bkramer
klimek
rnk
Summary

With that patch applied my 32-bit libclang.dll built with mingw does not crash anymore
unit tests are not affected by that change

Connected to
https://bugs.llvm.org/show_bug.cgi?id=32018

Diff Detail

yvvan created this revision.Jun 30 2017, 4:30 AM
yvvan edited the summary of this revision. (Show Details)
chapuni added a subscriber: chapuni.Jul 1 2017, 1:44 AM
erikjv added a subscriber: erikjv.
hans added a reviewer: rnk.Jul 18 2017, 6:22 AM
hans added a subscriber: hans.

From the bug, this is related to Richard's "r289413 - Add two new AST nodes to represent initialization of an array in terms of initialization of each array element", which broke MSVC builds due to under-alignment, which Reid addressed with "r289575 - Align EvalInfo in ExprConstant to avoid PointerUnion assertions".

Perhaps there's a problem with MinGW 4.9.2's alignas? Or is this https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78936 as Reid says in https://bugs.llvm.org/show_bug.cgi?id=32018#c8 ?

yvvan added a comment.Jul 18 2017, 6:27 AM

The same alignment in other places works fine. i don't know the source of that issue. Should be somewhere in gcc(mingw) but it's not my focus.
So this is a workaround that we can make (probably only for mingw builds)

rnk requested changes to this revision.Jul 19 2017, 2:37 PM

This doesn't seem like an acceptable workaround, surely this regresses functionality for arrays with more than UINT_MAX elements.

This revision now requires changes to proceed.Jul 19 2017, 2:37 PM
yvvan updated this revision to Diff 108620.Jul 28 2017, 3:41 AM

Provide a workaround without regression

rnk added inline comments.Jul 28 2017, 8:41 AM
lib/AST/ExprConstant.cpp
588

Surely this will fault on SPARC or ARM or other ISAs that care about alignment?

t.p.northover added inline comments.
lib/AST/ExprConstant.cpp
588

As well as being horribly undefined behaviour.

Ok, i will make safer solution...

yvvan updated this revision to Diff 108878.Jul 31 2017, 1:35 AM
yvvan marked 2 inline comments as done.

Make safe solution without regression

yvvan added a comment.Aug 4 2017, 1:34 AM

ping. Do you think this is ok now?

@rnk isn't this related to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78936
Assuming this is in fact a gcc bug this will most likely cause a regression with the standalone clang mingw toolchains we are on the verge of having.
This is of course assuming clang does not have the same regression gcc does.

rnk added a subscriber: gbiv.Aug 14 2017, 5:56 PM

I looked at the blame, and I added this alignment thing in rL289575 to deal with some PointerIntPair assertions. Those probably started in @gbiv's rL270781, which introduced a PointerIntPair<EvalInfo*,1,bool> field. I think we can just fix this problem by not being so clever. We can unpack the boolean and the pointer, and everything will work fine.

@rnk isn't this related to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78936
Assuming this is in fact a gcc bug this will most likely cause a regression with the standalone clang mingw toolchains we are on the verge of having.
This is of course assuming clang does not have the same regression gcc does.

It's possible, but nobody has debugged any of these crashes and shown the same ret $imm inconsistencies that I was pointing out in that bug. This seems unrelated at first.

rnk added a comment.Aug 14 2017, 6:19 PM

This shouldn't be necessary after rL310905.

yvvan abandoned this revision.Aug 16 2017, 12:06 AM
In D34873#841624, @rnk wrote:

This shouldn't be necessary after rL310905.