convert divcoeff.c to D rebooted #7714
Conversation
|
Thanks for your pull request, @WalterBright! Bugzilla referencesYour 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 locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + dmd#7714" |
WalterBright
force-pushed
the
divcoeff2
branch
from
6a87593 to
b096efb
Compare
| set -uexo pipefail | ||
|
|
||
| HOST_DMD_VER=2.072.2 # same as in dmd/src/posix.mak | ||
| HOST_DMD_VER=2.074.1 # same as in dmd/src/posix.mak |
There was a problem hiding this comment.
FYI: auto-tester needs to be bumped too, see the discussion on the previous PR and the NG discussion - especially Martin's reply.
|
In case the CI failures are unclear:
auto-tester still uses 2.068.2
Semaphore tests bootstrapping with the latest DMD, LDC + GDC releases. To repeat @ibuclaw from #6907:
|
It could be as simple as: #!/bin/bash
${HOST_DMD:-gdc} "${@/-betterC/}"Of course, you could also do this directly in the Makefile: ifeq (gdc, $(HOST_DMD_KIND))
BACK_FLAGS:=
else
BACK_FLAGS:=-betterC
endif |
|
WalterBright
force-pushed
the
divcoeff2
branch
from
b096efb to
f8569f9
Compare
WalterBright
force-pushed
the
divcoeff2
branch
3 times, most recently
from
2994e84 to
e0d2673
Compare
It's a mystery why this is failing. It links on my machine. Both names are in divcoeff.o. |
WalterBright
force-pushed
the
divcoeff2
branch
2 times, most recently
from
c979866 to
e4c4861
Compare
|
@wilzbach can you please have a look at divcoeff.o and see what the symbols in it are? |
WalterBright
force-pushed
the
divcoeff2
branch
from
e4c4861 to
f73a1be
Compare
|
Okay, I tried to add the workaround to optionally add Regarding the linker error that appears until 2.074: Looking at the
|
This fixes Jenkins.
This fixes SemaphoreCI. The remaining CIs (auto-tester + DAutoTest) fail due to mentioned The AppVeyor build failure is probably due to Maybe 2fe4d85 fixes this. |
Oh looking at auto-tester the linker errors between Linux and FreeBSD seem different. Maybe the root case is the same. |
|
The FreeBSD linker errors are: Since those are C mangled, how are they different on FreeBSD? |
Well, at least the good news is that the FreeBSD errors are the last CI errors and all other CI seem to be happy (though I'm not sure why SemaphoreCI isn't showing up here - it used to).
I have no idea as well. Might it be related to this: Maybe the old C++ compiler on FreeBSD used to have a different mangling scheme? |
Now it fails on Linux and macOS as well.
C doesn't have a mangling. |
|
Quite coincidentally I was planning to post in the getopt newsgroup thread that |
WalterBright
force-pushed
the
divcoeff2
branch
3 times, most recently
from
5e1af26 to
6bc0c42
Compare
As I understand it, FreeBSD is still using GNU's ld as its default linker (even in FreeBSD CURRENT). They're moving towards switching to using clang's as the default, but they're still working out the issues caused by that switch. |
|
This is failing on all autotester Posix platforms, not just FreeBSD. It's also not an ordering issue, because there should be only one divcoeff.o. |
|
Another idea I had is that it's maybe related to Lines 10 to 11 in c83e4b5 But it doesn't make a difference for me locally :/ FYI: the build script that is supposedly used by auto-tester is here: https://github.com/braddr/at-client/blob/master/src/do_build_dmd.sh |
|
Is no one able (I haven't tried yet) to reproduce this outside the tester? If it's failing as reliably and widely as indicated that shouldn't be difficult. Why are you working so hard to do this through the tester when it's almost certainly NOT the tester and easily reproduced outside it? |
|
Reproducing it outside the tester isn't really required; the fact that GNU ld (apart from The simplest solution is to add divcoeff.o to backend.a. |
@braddr nope. I even tried all LTSes of Ubuntu. Here's an example with the oldest that's not EOL yet: docker run --rm -ti ubuntu:14.04
apt-get update && apt-get install curl g++ make git unzip
git clone https://github.com/WalterBright/dmd
cd dmd
sed -i "s/2.074.1/2.068.2/" -i src/posix.mak
git checkout divcoeff2
make -f posix.mak AUTO_BOOTSTRAP=1Compiles flawlessly |
Order has nothing to do with this problem.
That's how it started out. With it in, out, explicitly before or after backend.a makes no difference to the error. |
|
Let's see what the state of this is:
What it smells like to me is there's another divcoeff.o on the autotester somewhere that it is picking up. Perhaps the Anyhow, this is proving impossible to debug remotely, and cannot be reproduced outside of the autotester. |
Ah, apologies, I must have jumped a line when looking at the old logs, and GitHub helpfully hid the earlier discussion. From experience, getting the order wrong would cause exactly that sort of hard-to-debug problem that only appears on certain linker versions when the stars align just the wrong way, though. |
|
Did any of you attempt building the target that the auto-tester runs: make MODEL=64 -f posix.mak auto-tester-build Fails for me quite easily and reproducible. And, notably, NOT specifying that target (going with whatever is the default) doesn't. A few minutes trying each of the targets in that dependency list and... it's cxx_unittest. |
|
Thanks for taking a look at this, @braddr ! But I'm not sure I'm understanding your results - is the cxx_unittest the problem? While you're at it, are you able to see the source of the problem? |
Yes. The ouput from the parallel jobs makes it a bit confusing, but it's displayed in this line
The command line is just missing divcoeff.o. Either add |
|
@rainers thanks! Trying it now... |
| @@ -1,258 +1,257 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
There was a problem hiding this comment.
I suspect you have converted newlines in the VS projects.
There was a problem hiding this comment.
Do you mean that vs projects have to have CRLF line endings? Sheesh.
There was a problem hiding this comment.
I don't think it's mandatory, but the next one who saves it from within VS will change the whole file again.
There was a problem hiding this comment.
The Jenkins failure is unrelated (see dlang/ci#190), so feel free to pull anytime you are ready Walter ;-)
| //ullong xxh = xh, xxl = xl, yyh = yh, yyl = yl; | ||
|
|
||
| assert(yh || yl); // no div-by-0 bugs | ||
| // assert(yh || yl); // no div-by-0 bugs |
There was a problem hiding this comment.
Why not use the dassert workaround for DAutoTest I proposed here?
#7714 (comment)
If the body is version(assert) it should behave like built-in assert? Alternatively it's really just 2.067 that needs special casing.
Or maybe @CyberShadow can update DAutoTest to use 2.068.2 as initial bootstrap compiler?
There was a problem hiding this comment.
I don't want to redo the asserts, as that doesn't scale to the rest of the back end. What's needed is to upgrade the boot compiler to 2.074. But that's for after this PR.
There was a problem hiding this comment.
as that doesn't scale to the rest of the back end.
Well, I thought it helps porting when the assert's are still enabled, but yeah this can be done in another PR
| $G/backend.a: $(G_OBJS) $(SRC_MAKE) | ||
| $(AR) rcs $@ $(G_OBJS) | ||
| $G/backend.a: $(G_OBJS) $(G_DOBJS) $(SRC_MAKE) | ||
| $(HOST_DMD_RUN) -lib -of$G/backend.a $(G_OBJS) |
There was a problem hiding this comment.
I think you can remove this line, now?
| $(AR) rcs $@ $(G_OBJS) | ||
| $G/backend.a: $(G_OBJS) $(G_DOBJS) $(SRC_MAKE) | ||
| $(HOST_DMD_RUN) -lib -of$G/backend.a $(G_OBJS) | ||
| $(HOST_DMD_RUN) -lib -of$G/backend.a $@ $(G_DOBJS) |
There was a problem hiding this comment.
Now that the problems has been identified this could be reverted to $(AR) rcs $@ $(G_OBJS)?
There was a problem hiding this comment.
Why use ar? dmd can create libraries just fine. I put it on 2 lines as the log file was cutting off the end of the line.
There was a problem hiding this comment.
Found a good reason for using $(AR) - gdmd doesn't support it.
-> #8111
|
Now that it's working, how about getting this pulled? |
|
So let's see what CIs break now ... |
9 participants
Reboot of #6907 because
git rebasesimply would not work with files that were renamed.