Skip to content

runnable/noreturn2.d: extern(C) functions have no defined order of parameter evaluation#13386

Closed
ibuclaw wants to merge 1 commit intodlang:masterfrom
ibuclaw:fix-tests
Closed

runnable/noreturn2.d: extern(C) functions have no defined order of parameter evaluation#13386
ibuclaw wants to merge 1 commit intodlang:masterfrom
ibuclaw:fix-tests

Conversation

@ibuclaw
Copy link
Member

@ibuclaw ibuclaw commented Dec 5, 2021

This test doesn't make any sense, there is no defined order that parameters are evaluated in when it comes to non-externD code -> make the test dmd-only.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @ibuclaw!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#13386"

@ibuclaw ibuclaw changed the title runnable/noreturn2.d: extern(C) functions have no defined order of evaluation runnable/noreturn2.d: extern(C) functions have no defined order of parameter evaluation Dec 5, 2021
@thewilsonator
Copy link
Contributor

Why not just remove the test? baking weird dmd-isms into the test suite doesn't really sound like a good idea. Also, spec update?

cc @kinke ?

@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 5, 2021

Why not just remove the test? baking weird dmd-isms into the test suite doesn't really sound like a good idea. Also, spec update?

I imagine it's just a bug in dmd because it returns early, before the left-to-right order swapping is done. I'm not going to touch that code though.

The spec for parameter evaluation is that it is implementation defined (10.2.7).

DIP1034 says (under expressions) "The order of evaluation of operands in binary expressions / parameter lists is defined to be from left to right." - but this cannot contradict or overrule 10.2.7.

@kinke
Copy link
Contributor

kinke commented Dec 5, 2021

The spec for parameter evaluation is that it is implementation defined (10.2.7).

WTF? Does GDC evaluate the args right-to-left for non-extern(D)?!

@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 5, 2021

The spec for parameter evaluation is that it is implementation defined (10.2.7).

WTF? Does GDC evaluate the args right-to-left for non-extern(D)?!

No, it simply doesn't enforce any order. It must be noted however that only x86 pushes arguments in reverse, and so is RTL when the back-end target expands the expression.

@kinke
Copy link
Contributor

kinke commented Dec 5, 2021

In LDC at least, evaluation of argument expressions is guaranteed to be LTR, decoupled from the order in which they are ultimately passed low-level wise. I expected this to be true for all compilers, and so the spec paragraph needing a correction; not defining that order for non-extern(D) callees is a bad joke IMO.

@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 5, 2021

In LDC at least, evaluation of argument expressions is guaranteed to be LTR, decoupled from the order in which they are ultimately passed low-level wise. I expected this to be true for all compilers, and so the spec paragraph needing a correction; not defining that order for non-extern(D) callees is a bad joke IMO.

IIRC array operands break if you evaluate extern(C) functions LTR (there's still an open bug for ARM that facebook put a bounty on, but @WalterBright blocked fixing the order of evaluation being changed due the subsequent increase in number of scratch registers it'll need to call said functions (or some optimization concern around that area) and I believe there is a workaround in the testsuite so that A()[] ~= B()[] ~ C()[] accepts both orders in the meantime). So are you really sure it's always LTR?

@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 5, 2021

there's still an open bug for ARM that facebook put a bounty on, but @WalterBright blocked fixing the order of evaluation being changed due the subsequent increase in number of scratch registers it'll need to call said functions (or some optimization concern around that area)

See #4035

@kinke
Copy link
Contributor

kinke commented Dec 5, 2021

So are you really sure it's always LTR?

I do recall some special cases for array ops in the past, but can't find any anymore in current LDC source. I guess they were gotten rid of when templatizing the arrayops; there's no extern in core.internal.array.operations, and the eval order seems to be regular LTR in https://run.dlang.io/is/7l9rBz. - Anyway, they were/are special compiler lowerings, not a regular D function call, so I don't see why this would have to be accounted for in the spec.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Dec 7, 2021

@ibuclaw @kinke is this good to go?

@kinke
Copy link
Contributor

kinke commented Dec 7, 2021

This shouldn't be required.

@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 7, 2021

This shouldn't be required.

I can make support for this the default, but perhaps the spec could be updated. :-)

@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 7, 2021

Anyway, they were/are special compiler lowerings, not a regular D function call, so I don't see why this would have to be accounted for in the spec.

Because the front end lowered it as a regular C function call, we could not see that it was disparate from a C function called from user code. The testsuite enforced that all parameters to said function was evaluated RTL.

Anyway I tried and failed to force this to be fixed in 2012, if this now works, then all the better.

@ibuclaw ibuclaw closed this Dec 9, 2021
@ibuclaw ibuclaw deleted the fix-tests branch December 9, 2021 00:40
@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 9, 2021

Latest merge with dmd master made ordered arguments the default, not just for extern(D).

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.

5 participants