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

Merge 2.074 front-end + libs #2076

Merged
merged 28 commits into from Jul 27, 2017
Merged

Merge 2.074 front-end + libs #2076

merged 28 commits into from Jul 27, 2017

Conversation

kinke
Copy link
Member

@kinke kinke commented Apr 22, 2017

No description provided.

@@ -179,6 +190,13 @@ struct Target
realalignsize = 2;
reverseCppOverloads = true;
c_longsize = 4;
if (ptrsize == 4)
Copy link
Member

Choose a reason for hiding this comment

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

Let's version this out.

Copy link
Member Author

Choose a reason for hiding this comment

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

This code is only used by DMD, we have our own Target struct at the top of the file.

return 0;
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This will need to be checked, i.e., whether a proper fix preventing the same field from being added multiple times to the fields list has made it into upstream 2.074.

@kinke
Copy link
Member Author

kinke commented Apr 22, 2017

First upstream PR: dlang/druntime#1815

@kinke
Copy link
Member Author

kinke commented Apr 22, 2017

Linux: 4 distinct failing unittest modules left, the rest (dmd-testsuite, lit-tests, LDC unittests) passes.

// skip over casts
auto ce = cs->exp;
while (ce->op == TOKcast)
ce = static_cast<CastExp *>(ce)->e1;
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 any chance that this drops a narrowing conversion? (Sorry if this is totally off, haven't studied the code closely.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. It's only used to check for a nested VarExp; cs->exp, the original expression, is evaluated in all cases (variable/constant).

@kinke
Copy link
Member Author

kinke commented Apr 23, 2017

2nd upstream PR: dlang/druntime#1816

@kinke
Copy link
Member Author

kinke commented Apr 23, 2017

Next one: dlang/phobos#5345

@kinke
Copy link
Member Author

kinke commented Apr 23, 2017

Another one: dlang/phobos#5346

@kinke
Copy link
Member Author

kinke commented Apr 23, 2017

Update:

  • There are mangling issues when using LDC 0.17.2 as host compiler (Travis LLVM 3.6 job).
  • OSX isn't tested anymore due to Travis OSX doesn't like LDC 1.2 as host compiler #2077, but the previous druntime compile error should be fixed (nogc fiber migration check).
  • Linux and Windows only have 2 failing unittest modules left:
    • A new core.memory unittest fails in release only (-O2 and higher), apparently because pureMalloc(size_t.max - 2) doesn't return null. I quickly checked the IR for a (working) stand-alone test, and both non-optimized and optimized IR looked fine.
    • std.math fails a bunch of new ieeeFlags checks. We already disabled a few of those (right above) due to potential instruction reordering (see std.math ieeeFlags cannot be reliably tested #888), but these new tests are more elaborate (using delegates), so I don't think LLVM interferes here even for debug builds, but I haven't digged deeper.

Conflicts:
	runtime/druntime
	runtime/phobos
@kinke
Copy link
Member Author

kinke commented May 11, 2017

With ldc-developers/phobos@2220a3a, all std.math-debug tests work on my Win64 box except for a rounding test.

@kinke
Copy link
Member Author

kinke commented May 15, 2017

Alright, first green runs - all but the 2 Travis -DBUILD_SHARED_LIBS=ON jobs should pass now (they seem to be using wrong filenames for the shared stdlibs or something like that).

Conflicts:
	runtime/druntime
	runtime/phobos
@kinke
Copy link
Member Author

kinke commented Jun 10, 2017

Upgraded to 2.074.1 and synced with master.

In case the host real_t is double (e.g., for MSVC hosts).
Conflicts:
	runtime/druntime
Conflicts:
	tests/d2/dmd-testsuite
@kinke
Copy link
Member Author

kinke commented Jul 13, 2017

As to the final issue: not even a Phobos hello-world program can be linked when using the shared runtime libs due to undefined members of this nested struct:

echo "import std.stdio; void main() { writefln(\"Hello LDC2 %d\", (void*).sizeof); }" > hello.d
bin/ldc2 hello.d -link-debuglib
/home/martin/build-ldc/lib/libphobos2-ldc-debug.so: undefined reference to `_D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result5emptyMFNaNbNdNiNfZb'
/home/martin/build-ldc/lib/libphobos2-ldc-debug.so: undefined reference to `_D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result5frontMFNaNbNdNiNfZa'
/home/martin/build-ldc/lib/libphobos2-ldc-debug.so: undefined reference to `_D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result6lengthMFNaNbNdNiNfZm'
/home/martin/build-ldc/lib/libphobos2-ldc-debug.so: undefined reference to `_D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result8popFrontMFNaNbNiNfZv'
/home/martin/build-ldc/lib/libphobos2-ldc-debug.so: undefined reference to `_D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZS3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result'
collect2: error: ld returned 1 exit status
Error: /usr/bin/gcc failed with status: 1

Any ideas why only the shared libs are affected? No public visibility or something like that? I know nothing about shared libs on Linux, but the template seems to be instantiated in Phobos (=> #2168).

Conflicts:
	runtime/druntime
Conflicts:
	runtime/druntime
	tests/d2/dmd-testsuite
@kinke
Copy link
Member Author

kinke commented Jul 21, 2017

Static:

$ readelf -Ws build-ldc/lib/libphobos2-ldc.a | grep _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result
   886: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result5emptyMFNaNbNdNiNfZb
   887: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result5frontMFNaNbNdNiNfZa
   888: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result6lengthMFNaNbNdNiNfZm
   889: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result8popFrontMFNaNbNiNfZv

vs. shared:

$ readelf -W --dyn-syms build-ldc/lib/libphobos2-ldc-shared.so | grep _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result
    57: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result5emptyMFNaNbNdNiNfZb
   233: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result5frontMFNaNbNdNiNfZa
   255: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result6lengthMFNaNbNdNiNfZm
   298: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result8popFrontMFNaNbNiNfZv
$ readelf -Ws build-ldc/lib/libphobos2-ldc-shared.so | grep _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result
    57: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result5emptyMFNaNbNdNiNfZb
   233: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result5frontMFNaNbNdNiNfZa
   255: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result6lengthMFNaNbNdNiNfZm
   298: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result8popFrontMFNaNbNiNfZv
  3318: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result5emptyMFNaNbNdNiNfZb
  9136: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result5frontMFNaNbNdNiNfZa
  9696: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result6lengthMFNaNbNdNiNfZm
 11076: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _D3std4conv47__T7toCharsVii10TaVE3std5ascii10LetterCasei1TkZ7toCharsFNaNbNiNfkZ6Result8popFrontMFNaNbNiNfZv

Edit: readelf -s ...-shared.so displays both .dynsym and .symtab tables, that's why the symbols are listed twice.

@kinke
Copy link
Member Author

kinke commented Jul 21, 2017

Oh and btw, both shared and static Phobos libs consist of the same D object files.
Any suggestions? I'm stuck. Happens on OSX as well, not just Linux.

@JohanEngelen
Copy link
Member

So the function is not codegenned, I've run into similar issues with Weka's codebase where some templates are culled too aggressively. But also because of failing to deduce attributes in certain cases but not in others: https://issues.dlang.org/show_bug.cgi?id=17541.
Can you check if there isn't another symbol defined for that function but without the attributes?

@kinke
Copy link
Member Author

kinke commented Jul 21, 2017

So the function is not codegenned

But then why is the linker not complaining when feeding it the static lib with exactly the same D objects?

@JohanEngelen
Copy link
Member

Conjecture: in the static case, the linker sees all symbols and knows it can strip the symbol in which the undef thing is referenced. In the sharedlib case, perhaps the linker cannot make as many assumptions, because the shared lib could be replaced with another and then behavior could be different when certain symbols are available or not? Really, I don't know.

@kinke
Copy link
Member Author

kinke commented Jul 21, 2017

Thanks anyway - I tested the needsCodegen() hack from #2168, commenting out this, and hello-world can then be linked successfully.

@JohanEngelen
Copy link
Member

JohanEngelen commented Jul 21, 2017

This is the first time I see that happening in Phobos stuff. That's very good news: see if it happens with dmd too, dustmite and submit bugreport. The bug with Weka source has been a bit elusive so far. (or I've lost track in the confusion of the weird complex bugs I submitted...)

@kinke
Copy link
Member Author

kinke commented Jul 21, 2017

DMD 2.074.0 has no problems with linking the above hello-world with its combined (druntime + Phobos) libphobos2.so on Linux x64. That would have been shocking. ;)

@JohanEngelen
Copy link
Member

lol heh true. (thought they are doing static linking) Very strange that our front-end culls, and theirs doesn't though....

@kinke
Copy link
Member Author

kinke commented Jul 21, 2017

Very strange that our front-end culls, and theirs doesn't though

I have a strong assumption - they seem to build druntime & Phobos in a single command line, all D files at once: https://github.com/dlang/phobos/blob/master/posix.mak#L321
Whereas we compile each object separately. And that culling basically checks whether a template is (also) instantiated in some imported non-root module. So if the complete importable source is specified on the command line, this problem can't occur - everything is in a root module.

@kinke
Copy link
Member Author

kinke commented Jul 21, 2017

So this should basically be due to (supposedly fixed) https://issues.dlang.org/show_bug.cgi?id=2500 or some more sophisticated variant of it.

@kinke
Copy link
Member Author

kinke commented Jul 21, 2017

thought they are doing static linking

They do by default but ship with static and shared libs, so I caught the linker line via -v and linked against the shared lib manually.

@kinke
Copy link
Member Author

kinke commented Jul 22, 2017

All green now after the template culling hack.

CMakeLists.txt Outdated
@@ -817,6 +819,9 @@ add_subdirectory(tests)

install(PROGRAMS ${LDC_EXE_FULL} DESTINATION ${CMAKE_INSTALL_PREFIX}/bin)
install(PROGRAMS ${LDMD_EXE_FULL} DESTINATION ${CMAKE_INSTALL_PREFIX}/bin)
if(NOT CMAKE_INSTALL_PREFIX STREQUAL "/usr")
Copy link
Member

Choose a reason for hiding this comment

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

Huh? This smells massively.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to install the bash script system-wide. It's more of an internal script by itself, useful for local builds and release packages, but not system-wide availability IMO.
Correct context: #2198

@kinke
Copy link
Member Author

kinke commented Jul 27, 2017

I created a test branch merge-2.074-2 with the template culling hack reverted and #2231 merged. As expected, this also fixes the linking issues with shared Phobos. The Phobos unittest runners can also be linked successfully although the runtime modules are compiled separately. See https://travis-ci.org/ldc-developers/ldc/builds/257913800. And both Semaphore jobs successfully compiled Phobos; that may have been a fluke or the #2231 Semaphore issue has been fixed in 2.074.

I'd prefer using #2231 (maybe merged as part of 2.074 if the Semaphore issue reproducibly vanishes) and expose the weakened template culling hack as command-line option. Thoughts?

@kinke
Copy link
Member Author

kinke commented Jul 27, 2017

if the Semaphore issue reproducibly vanishes

Well, no Phobos compilation failures after 10 jobs, so I think it's safe to assume 2.074 gets rid of that last issue of #2231.
[The random failures when linking the testrunners are due to parallel ninja invokations when running the build-*-test-runner* CTest tests in parallel. Unfortunately, Semaphore doesn't support an in-source config file, so I can't switch the global config to ninja all all-test-runners as long as #2231 isn't merged.]

Conflicts:
	runtime/druntime
	runtime/phobos
@kinke kinke merged commit 7a9960f into master Jul 27, 2017
@kinke kinke deleted the merge-2.074 branch July 27, 2017 22:16
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.

None yet

3 participants