Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Win64 ABI wrt. passing structs > 64 bit #756

Merged
merged 1 commit into from
Oct 21, 2014

Conversation

kinke
Copy link
Member

@kinke kinke commented Oct 21, 2014

This fixes #754.

// rewritten as integers
return (t->ty == Tstruct || t->ty == Tsarray) && !canRewriteAsInt(t);
// FIXME: LLVM doesn't seem to support ByVal on Win64 yet
//return isPassedWithByvalSemantics(t);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to not call isPassedWithByvalSemantics()? The sole effect is that the byval attribute is set (which has no effect because the argument is rewritten). The other ABI implementations use the same approach.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No particular reason, I just wasn't sure whether byval would really be ignored if the argument is rewritten.

@kinke
Copy link
Member Author

kinke commented Oct 21, 2014

Now we're down to 63 errors in druntime/phobos (release and debug) on Win64:

core.thread (SEGFAULT)
core.time (SEGFAULT)
rt.lifetime (SEGFAULT)
rt.minfo (SEGFAULT)
std.algorithm (SEGFAULT)
std.array (SEGFAULT)
std.complex (Failed)
std.concurrency (SEGFAULT)
std.conv (SEGFAULT)
std.csv (Failed)
std.datetime (SEGFAULT)
std.exception (SEGFAULT)
std.getopt (SEGFAULT)
std.math (Failed)
std.numeric (Failed)
std.outbuffer (Failed)
std.parallelism (SEGFAULT)
std.path (SEGFAULT)
std.process (SEGFAULT)
std.regex (SEGFAULT)
std.socket (SEGFAULT)
std.stdio (SEGFAULT)
std.string (SEGFAULT)
std.uni (SEGFAULT)
std.uri (SEGFAULT)
std.variant (Failed)
std.xml (SEGFAULT)
std.zlib (SEGFAULT)
std.container.array (SEGFAULT)
std.net.isemail (SEGFAULT)
std.internal.math.errorfunction (Failed)
std.internal.math.gammafunction (Failed)
core.time-debug (SEGFAULT)
rt.lifetime-debug (SEGFAULT)
rt.minfo-debug (SEGFAULT)
std.algorithm-debug (SEGFAULT)
std.array-debug (SEGFAULT)
std.complex-debug (Failed)
std.concurrency-debug (SEGFAULT)
std.conv-debug (SEGFAULT)
std.csv-debug (Failed)
std.datetime-debug (SEGFAULT)
std.exception-debug (SEGFAULT)
std.getopt-debug (SEGFAULT)
std.json-debug (Failed)
std.math-debug (Failed)
std.numeric-debug (Failed)
std.outbuffer-debug (Failed)
std.parallelism-debug (SEGFAULT)
std.path-debug (Failed)
std.process-debug (SEGFAULT)
std.regex-debug (SEGFAULT)
std.socket-debug (SEGFAULT)
std.stdio-debug (SEGFAULT)
std.uni-debug (SEGFAULT)
std.uri-debug (SEGFAULT)
std.variant-debug (SEGFAULT)
std.xml-debug (SEGFAULT)
std.zlib-debug (SEGFAULT)
std.container.array-debug (SEGFAULT)
std.net.isemail-debug (SEGFAULT)
std.internal.math.errorfunction-debug (Failed)
std.internal.math.gammafunction-debug (Failed)

I.e., this fixes 15-20 errors. :)

redstar added a commit that referenced this pull request Oct 21, 2014
Fix Win64 ABI wrt. passing structs > 64 bit
@redstar redstar merged commit 8b43678 into ldc-developers:master Oct 21, 2014
@redstar
Copy link
Member

redstar commented Oct 21, 2014

Thanks!

@Trass3r
Copy link
Contributor

Trass3r commented Oct 21, 2014

Did you build with assertions enabled?

@kinke
Copy link
Member Author

kinke commented Oct 21, 2014

Yes I did, getting the same problem now. It's due to the byval attribute, see the outdated line diff comments. Simply returning false in passByVal() works.

@Trass3r
Copy link
Contributor

Trass3r commented Oct 21, 2014

Hmm ok first I thought it's cause of the llvm update but returning false makes the assertions go away indeed.

@kinke
Copy link
Member Author

kinke commented Oct 21, 2014

Hotfix: #757

@kinke
Copy link
Member Author

kinke commented Oct 21, 2014

@Trass3r Note that you'll run into ldc-developers/druntime@2c2c8ca#commitcomment-8248875 as well. Simply remove the INTRINSICS_FROM_302 version guard for the moment.

@Trass3r
Copy link
Contributor

Trass3r commented Oct 21, 2014

Do you also get empty "application error" message boxes with the crashing unittests?

@kinke
Copy link
Member Author

kinke commented Oct 21, 2014

Hmm, I get the normal app-crashed Windows dialog on segfault (with the option to close or debug it) and 2-3 message boxes from the MSVCRT ("abort() called" iirc). Is that what you mean?

@Trass3r
Copy link
Contributor

Trass3r commented Oct 21, 2014

With the former assertions in ldc I get the usual close or debug dialog. But the test runner crashes suddenly (?) only produce a messagebox with title Application Error, an icon, no description and no text on the ok button. Maybe some /MT weirdness.

@kinke
Copy link
Member Author

kinke commented Oct 21, 2014

Good guess, I've seen some warnings about /MT overriding /MTd or something like that. I guess your weird dialog should be the MSVCRT dialog about abort().
/edit: Although I've seen dialogs such as these in case one runs out of memory. Note that the 6 parallel LDC2 processes using ninja (on my system: 4 cores + 2 extra tasks) can eat up quite huge amounts, iirc, std.algorithm-unittest alone takes ~3.5 gigs.
And the build time for the tests has increased considerably (+60% or so, especially for phobos-unittest in release mode) since real == double (I don't recall since when exactly). I guess it may have to do with CTFE which was previously blocked by inline assembly in std.math.

@Trass3r
Copy link
Contributor

Trass3r commented Oct 22, 2014

Yeah one problem was I modified the CXX flags for static C runtime, but not the C flags.
The other one was the curl library.

@Trass3r
Copy link
Contributor

Trass3r commented Oct 22, 2014

Also see #758

@kinke kinke deleted the abi branch August 24, 2017 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failing std.bitmanip unittest on Win64: peek() wrongly consumes the range
3 participants