Created attachment 6 [details] reduced test case Test case attached. Reduced from runnable/arrayop.d Compile & run: gdc arrayop.d -o arrayop.exe ./arrayop.exe
Created attachment 7 [details] system / compier information
You may think so, but this is actually not a bug, just a little known example of an x86-specific quirk. In D functions (thus, extern D) - everything should be evaluated left to right (LTR). And I am pretty confident for most cases, GDC gets this correct in the codegen despite pulling an arm or two in the process to get it done. For extern C functions you are left up to the order of which the underlying architecture pushes arguments on the stack. As it just so happens, x86/x86_64 is right to left (RTL), aka PUSH_ARGS_REVERSED. So expecting BCA makes sense as: A()[] = B()[] + C()[]; Gets turned into: _arraySliceSliceAddSliceAssign_f(A[], C[], B[]); Hence why it asserts that the result is "BCA". However, ARM pushes arguments on the stack LTR, so we should instead expect the result of the array operation to be "ACB"
Just leaving some IRC logs here to record my opinion on the matter: 20:52:37 < Zor> ibuclaw: in my opinion, the language should make strong guarantees about this stuff that compilers must follow, regardless of arch quirks 20:53:05 < ibuclaw> extern(D) makes strong guarantees. 20:53:28 < ibuclaw> extern(C) doesn't, which is why the testcase is fairly bogus. 20:55:44 < Zor> ibuclaw: I don't see why the compiler can't make the same guarantees about extern (C) functions too 20:56:48 < Zor> ibuclaw: the compiler would have to do some extra work to ensure LTR evaluation order, but that shouldn't be impossible 20:58:08 < Zor> ibuclaw: I especially think it's worth looking into because there's nothing even mentioning C linkage/ABI in this piece of D code, which makes the gotcha even more ridiculous 20:58:57 < ibuclaw> Zor, the problem is there would be code out there that relies on the already existing behaviour, sadly enough. 20:59:28 < Zor> yes, but there's no point in caring about that 20:59:41 < Zor> TDPL says left-to-right, always 20:59:53 < Zor> it'll break code, but we need to clean up the mess sooner rather than later 21:00:51 < ibuclaw> Zor, does it explicitly say 'all functions, even extern(C)' ? :^) 21:01:34 < Zor> All arguments are evaluated left to right 21:01:34 < Zor> before fun gets invoked. 21:01:50 < Zor> that's in the description of the function call operator 21:01:59 < Zor> so it's regardless of linkage, I'd say 21:02:28 < Zor> besides, I think code relying on RTL evaluation will be very rare 21:02:34 < Zor> LTR is what people intuitively expect
For reference: Related discussion in D newsgroup: http://forum.dlang.org/thread/bniaxycuguviwfdtojzf@forum.dlang.org
$200 bouty placed: https://www.bountysource.com/issues/1407206-arm-runnable-arrayop-d-fails-wrong-execution-order
Iain, do you want to look into this again? I read some of the old discussions and the dlang.org pull request and to summarize: * All functions should be evaluated LTR, even extern(C) * Array OPs expect extern(C) functions to be evaluated RTL * DMD backend does extern(C) functions RTL So in order to fix this, we'd have to evaluate extern(C) function LTR (easy, as the hard work has already been done for D functions) and change array ops to work with LTR extern(C) functions (needs quite some changes in the frontend and druntime, but it's not hard to implement). However, as soon as we change array ops in the frontend we break RTL backends(dmd, probably ldc). And fixing these backends as well is probably out of scope. We're probably not familiar with these and it might be a bad idea from a legal perspective as well? However, I think we could do this: * Make extern(C) functions LTR in GDC * Make array ops expect LTR C functions in the frontend and druntime * Introduce a Target::isExternCLTR flag * Add ~10 lines of special code for backends which use RTL: Just evaluate the parameters to VarDecls explicitly before passing them to the C function for array ops This way everything works for all compilers and as soon as all compilers have LTR evaluation we could remove Target::isExternCLTR and the extra code.
Another link for reference: Pull request for the spec which demands LTR evaluation: https://github.com/D-Programming-Language/dlang.org/pull/6
And one more link: DMD bug: https://d.puremagic.com/issues/show_bug.cgi?id=6620
LTR is already enforced for LINKd functions. See d-codegen.cc(d_build_call) // Evaluate the argument before passing to the function. // Needed for left to right evaluation. if (tf->linkage == LINKd && TREE_SIDE_EFFECTS (targ)) { /* ... */ }
(In reply to comment #6) > Iain, do you want to look into this again? > > I read some of the old discussions and the dlang.org pull request and to > summarize: > * All functions should be evaluated LTR, even extern(C) > * Array OPs expect extern(C) functions to be evaluated RTL > * DMD backend does extern(C) functions RTL > > So in order to fix this, we'd have to evaluate extern(C) function LTR (easy, as > the hard work has already been done for D functions) and change array ops to > work with LTR extern(C) functions (needs quite some changes in the frontend and > druntime, but it's not hard to implement). However, as soon as we change array > ops in the frontend we break RTL backends(dmd, probably ldc). And fixing these > backends as well is probably out of scope. We're probably not familiar with > these and it might be a bad idea from a legal perspective as well? > Not sure about a legal perspective, but if there's something fundamentally wrong with language semantics and we send a fix upstream to the frontend, I'd rely on the respective backend devs to be aware of it, and fix the problem themselves. It should be easy enough, as all you are doing is caching values in the codegen before passing. IMO, I think Andrei would agree that all parameters should be LTR evaluated regardless of LINK declarations. To summarise the current situation: extern(D) int foo(int a, ...); extern(C) int bar(int a, ...); foo(i++, i++, i++); // 1, 2, 3 - Always! bar(i++, i++, i++); // 3, 2, 1 - But only on x86!
OK. So do you want to fix this or shall I?
Yes, I agree all parameters should be evaluated LTR. In fact LTR is more general than that - in an assignment e1 = e2, e1 should be evaluated first. Also in a computed function call expr1(expr2), expr1 should be evaluated first.
(In reply to comment #12) > Yes, I agree all parameters should be evaluated LTR. In fact LTR is more > general than that - in an assignment e1 = e2, e1 should be evaluated first. > Also in a computed function call expr1(expr2), expr1 should be evaluated first. OK, let's break DMD! >:)
(In reply to comment #11) > OK. So do you want to fix this or shall I? I'm pretty tied up getting gdb D into shape, and working with a publishing company in reviewing a new D2 book, and doing the 2.065 merge - which depends on preliminary stuff such as eventual Visitor conversions almost *all* our glue code, and re-implement dfrontend/builtins.c into the glue cause the new revision of it in 2.065 is not GDC-friendly... The long route in GDC would be (doing this in my head): 1. d-codegen.cc(d_build_call): Lift the LINKd restriction in which caches arguments in a LTR fashion. 2. dfrontend/arrayops.c(AssignExp::buildArrayIdent, BinAssignExp::buildArrayIdent, AssignExp::buildArrayLoop, BinAssignExp::buildArrayLoop): Evaluate assign expressions left to right 3. Reverse the parameters for all arrayops in druntime. // a[] = b[] op value: (T[] a, T value, T[] b) => (T[] b, T value, T[] a) // a[] = b[] op c[] (T[] a, T[] c, T[] b) => (T[] b, T[] c, T[] a) // a[] += value (T[] a, value) => (value, T[] a) // a[] += b[] (T[] a, T[] b) => (T[] b, T[] a) 4. Test! 5. Push upstream, where someone will work out how to fix it in DMD. Unless Mr. Andrei thinks that a[] = b[] op c[] should be evaluated in order of: a[] -> b[] -> c[] Instead of keeping the current behaviour: b[] -> c[] -> a[] In which case, just do items number #1, #4 and #5 in the list above. With the additional step of at stage #4 fix old tests that relied on the old behaviour of evaluation. Regards Iain.
OK, here's the fix for current GDC: https://github.com/jpf91/GDC/commits/fixOrder GDC/frontend changes https://github.com/jpf91/GDC/commit/5bc609b3fdc42d8df46118ed19ac8b582fe47040 Druntime changes https://github.com/jpf91/GDC/commit/3517843d79e7259b91f160404448423c26395e79 Changes to old tests + new tests: https://github.com/jpf91/GDC/commit/161e2dd60414226bbb6e8209816c76f525d581dd The next steps are: * Porting this to dmd, then asking the dmd maintainers to adapt their backend to evaluate extern(C) functions and assignments LTR. * Waiting till the 2.065 frontend has been merged into GDC * Then backport these changes to GDC
Almost forgot: Tested & working on X86_64 and ARM :-)
http://forum.dlang.org/thread/lhe96q$27ua$1@digitalmars.com