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

partial fix for Issue 5570 #3372

Merged
merged 1 commit into from
Mar 11, 2014
Merged

partial fix for Issue 5570 #3372

merged 1 commit into from
Mar 11, 2014

Conversation

WalterBright
Copy link
Member

Partial in that it doesn't solve the problem for returning structs of sizes 3,5-7,9-15

https://d.puremagic.com/issues/show_bug.cgi?id=5570

@WalterBright
Copy link
Member Author

Hmm, does EXTRA_C_SOURCES work?

@ibuclaw
Copy link
Member

ibuclaw commented Mar 10, 2014

Isn't argtypes a visitor yet? I was looking forward to booting the entire file from gdc dfrontend sources.

@yebblies
Copy link
Member

@WalterBright There is no EXTRA_C_SOURCES, only EXTRA_CPP_SOURCES.

@ibuclaw Yes it is.

case 15:
if (!global.params.is64bit)
goto Lmemory;
t1 = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

t1 is already null by the looks of it.

Copy link
Member

Choose a reason for hiding this comment

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

okay for the sake of symmetry

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but I put that in because the other cases set t1.

@andralex
Copy link
Member

Why the fail on win64?

@WalterBright
Copy link
Member Author

I disabled the Win64 tests for now. The Win64 stuff is totally different, and these tests had never been run on it before, so this is not a regression.

@braddr
Copy link
Member

braddr commented Mar 10, 2014

Does that mean that the win64 abi code gen is also broken and this bug report is incorrect in marking it as linux only? And that this fix is even more partial? I'm just asking so that we don't loose track of additional bugs that might have just been revealed. And any test that's disabled due to bugs should have a bug filed (probably with a severity in the blocker, critical, or major range) and a comment in the test code pointing at that bug report, and probably should emit a "disabled on xx due to yy" sort of comment. Anything to draw attention to a disabled test. Otherwise, it'll just get lost in the noise of all the open bugs.

@WalterBright
Copy link
Member Author

@braddr yes it means that with some structs the Win64 ABI is not followed.

The Win64 ABI is very, very different and would need a completely different fix. I think it merits a separate bugzilla issue. http://d.puremagic.com/issues/show_bug.cgi?id=12343

@andralex
Copy link
Member

Thanks! This is helping FB Thrift work.

andralex added a commit that referenced this pull request Mar 11, 2014
@andralex andralex merged commit a707998 into dlang:master Mar 11, 2014
@WalterBright WalterBright deleted the fix5570 branch March 11, 2014 00:26
@JackStouffer
Copy link
Member

This pull request introduced a regression https://issues.dlang.org/show_bug.cgi?id=15455

ibuclaw pushed a commit to ibuclaw/dmd that referenced this pull request Jul 10, 2022
ibuclaw pushed a commit to ibuclaw/dmd that referenced this pull request Jul 10, 2022
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.

6 participants