[WIP] Attempt to fix #2497 #2503

Hardcode84 · 2018-01-14T12:09:49Z

I have no idea how OSX infrastructure works, just some copypaste and stackowerflow-driven development.

Also, if we have both dynamic compilation and asan enabled, duplicating entries will be added to -rpath, is this ok?

JohanEngelen · 2018-01-14T17:19:23Z

driver/linker-gcc.cpp

@@ -343,6 +343,14 @@ void ArgsBuilder::build(llvm::StringRef outputPath,
  if (opts::enableDynamicCompile) {
    args.push_back("-lldc-jit-rt");
    args.push_back("-lldc-jit");
+    if (global.params.targetTriple->isOSDarwin()) {


make this a little more descriptive, with a bool linkShared or something. Is Darwin the only place where shared JIT RT can be used?

(also it's nicer to put all linker flags related to enableDynamicCompile in a separate function, like addASanLinkFlags)

Is Darwin the only place where shared JIT RT can be used?

rpath works differently on OSX and on linux and @executable_path is only supported on OSX.

JohanEngelen · 2018-01-14T17:20:02Z

driver/linker-gcc.cpp

+
+      std::string jitLib = exe_path::prependLibDir("libldc_jit.dylib");
+      args.push_back("-rpath");
+      args.push_back(llvm::sys::path::parent_path(jitLib));


why not use exe_path::getLibDir ?

JohanEngelen · 2018-01-14T17:32:44Z

duplicating entries will be added to -rpath, is this ok?

For now, I think it's OK. I tested it and it doesn't give problems on my machine (OSX 10.13.2).
There is this: STEllAR-GROUP/hpx#1299 and openmm/openmm#295 .

Hardcode84 · 2018-01-16T18:22:44Z

Updated:

Moved to separate function
Added helper to avoid duplicating flags

JohanEngelen · 2018-01-16T22:37:59Z

driver/linker-gcc.cpp

@@ -70,6 +74,10 @@ class ArgsBuilder {
  virtual void addLdFlag(const llvm::Twine &flag1, const llvm::Twine &flag2) {
    args.push_back(("-Wl," + flag1 + "," + flag2).str());
  }
+
+  void addUniqueArgs(const std::vector<std::string>& arg);


nit: clang-format for the position of the &

JohanEngelen · 2018-01-16T22:38:10Z

driver/linker-gcc.cpp

@@ -495,6 +514,14 @@ void ArgsBuilder::addTargetFlags() {
  appendTargetArgsForGcc(args);
 }

+void ArgsBuilder::addUniqueArgs(const std::vector<std::string> &arg) {
+  assert(!arg.empty());
+  if (uniqueArgs.end() == uniqueArgs.find(arg)) {


nit: again the inverted comparison order ;-)

JohanEngelen · 2018-01-16T22:42:22Z

driver/linker-gcc.cpp

+
+  void addUniqueArgs(const std::vector<std::string>& arg);
+
+  std::set<std::vector<std::string>> uniqueArgs;


uniqueArgs are not read anywhere yet?

It used in addUniqueArgs. If arg isn't in uniqueArgs we add them to args and to uniqueArgs

Ah, indeed. I missed that it adds the args to both args and uniqueArgs.

Hardcode84 · 2018-01-17T15:43:15Z

Updated

JohanEngelen · 2018-01-17T21:38:42Z

is this still work-in-progress? LGTM.
@ThomasMader can you verify that this fixes #2497 ?

ThomasMader · 2018-01-20T10:49:48Z

Unfortunately it's not fixing it. "codegen/attr_assumeused.d" additionally fails now but that might be another problem with the base branch.
Rebasing the branch on top of 1.7.0 release tag should prevent such problems.

771: FAIL: LDC :: codegen/attr_assumeused.d (29 of 193) 771: ******************** TEST 'LDC :: codegen/attr_assumeused.d' FAILED ******************** 771: Script: 771: -- 771: /tmp/nix-build-ldcBuild-1.7.0.drv-3/ldc-v1.7.0-src/build/bin/ldc2 -c -output-ll -of=/private/tmp/nix-build-ldcBuild-1.7.0.drv-3/ldc-v1.7.0-src/build/tests/codegen/Output/attr_assumeused.d.tmp.ll /private/tmp/nix-build-ldcBuild-1.7.0.drv-3/ldc-v1.7.0-src/tests/codegen/attr_assumeused.d && FileCheck /private/tmp/nix-build-ldcBuild-1.7.0.drv-3/ldc-v1.7.0-src/tests/codegen/attr_assumeused.d < /private/tmp/nix-build-ldcBuild-1.7.0.drv-3/ldc-v1.7.0-src/build/tests/codegen/Output/attr_assumeused.d.tmp.ll 771: -- 771: Exit Code: 1 771: 771: Command Output (stdout): 771: -- 771: $ "/tmp/nix-build-ldcBuild-1.7.0.drv-3/ldc-v1.7.0-src/build/bin/ldc2" "-c" "-output-ll" "-of=/private/tmp/nix-build-ldcBuild-1.7.0.drv-3/ldc-v1.7.0-src/build/tests/codegen/Output/attr_assumeused.d.tmp.ll" "/private/tmp/nix-build-ldcBuild-1.7.0.drv-3/ldc-v1.7.0-src/tests/codegen/attr_assumeused.d" 771: # command stderr: 771: /private/tmp/nix-build-ldcBuild-1.7.0.drv-3/ldc-v1.7.0-src/tests/codegen/attr_assumeused.d(11): Error: undefined identifier assumeUsedin moduleldc.attributes771: /private/tmp/nix-build-ldcBuild-1.7.0.drv-3/ldc-v1.7.0-src/tests/codegen/attr_assumeused.d(15): Error: undefined identifierassumeUsedin moduleldc.attributes 771: 771: error: command failed with exit status: 1 771: 771: -- 771: 771: ********************

771: Failing Tests (16):
771:     LDC :: codegen/attr_assumeused.d
771:     LDC :: dynamiccompile/array.d
771:     LDC :: dynamiccompile/calls.d
771:     LDC :: dynamiccompile/classes.d
771:     LDC :: dynamiccompile/dump_handler.d
771:     LDC :: dynamiccompile/empty_jit_modules.d
771:     LDC :: dynamiccompile/globals.d
771:     LDC :: dynamiccompile/globals_types.d
771:     LDC :: dynamiccompile/lambdas.d
771:     LDC :: dynamiccompile/params_ctors.d
771:     LDC :: dynamiccompile/recursive_call.d
771:     LDC :: dynamiccompile/simple.d
771:     LDC :: dynamiccompile/struct_init.d
771:     LDC :: dynamiccompile/thread_local.d
771:     LDC :: dynamiccompile/throw.d
771:     LDC :: dynamiccompile/tls_workaround_opt.d

771: FAIL: LDC :: dynamiccompile/tls_workaround_opt.d (160 of 193) 771: ******************** TEST 'LDC :: dynamiccompile/tls_workaround_opt.d' FAILED ******************** 771: Script: 771: -- 771: /tmp/nix-build-ldcBuild-1.7.0.drv-3/ldc-v1.7.0-src/build/bin/ldc2 -enable-dynamic-compile -dynamic-compile-tls-workaround=0 -run /private/tmp/nix-build-ldcBuild-1.7.0.drv-3/ldc-v1.7.0-src/tests/dynamiccompile/tls_workaround_opt.d 771: /tmp/nix-build-ldcBuild-1.7.0.drv-3/ldc-v1.7.0-src/build/bin/ldc2 -enable-dynamic-compile -dynamic-compile-tls-workaround=1 -run /private/tmp/nix-build-ldcBuild-1.7.0.drv-3/ldc-v1.7.0-src/tests/dynamiccompile/tls_workaround_opt.d 771: -- 771: Exit Code: 2 771: 771: Command Output (stdout): 771: -- 771: $ "/tmp/nix-build-ldcBuild-1.7.0.drv-3/ldc-v1.7.0-src/build/bin/ldc2" "-enable-dynamic-compile" "-dynamic-compile-tls-workaround=0" "-run" "/private/tmp/nix-build-ldcBuild-1.7.0.drv-3/ldc-v1.7.0-src/tests/dynamiccompile/tls_workaround_opt.d" 771: # command stderr: 771: dyld: Library not loaded: libldc-jit.77.dylib 771: Referenced from: /private/tmp/nix-build-ldcBuild-1.7.0.drv-3/lit_tmp_Y5IInt/tls_workaround_opt-9829df1-e7c05f 771: Reason: image not found 771: Error: /private/tmp/nix-build-ldcBuild-1.7.0.drv-3/lit_tmp_Y5IInt/tls_workaround_opt-9829df1-e7c05f failed with status: -2 771: Error: message: Abort trap: 6 771: Error: program received signal 2 (Interrupt: 2) 771: 771: error: command failed with exit status: 2 771: 771: -- 771: 771: ********************

Hardcode84 · 2018-01-22T19:27:39Z

@ThomasMader can you, please, add instructions how to reproduce Nix setup and how to test specific branch.

ThomasMader · 2018-01-22T20:13:08Z

On a Mac you should be able to reproduce.

You need to checkout https://github.com/ThomasMader/nixpkgs/tree/dynamiccompile
Install nix on the machine with: curl https://nixos.org/nix/install | sh
cd into the repository and run: nix-build -K -A ldc
This will need some time because it needs to build the bootstrap compiler too because this changed package is not in the official binary caches.
It should fail as I reported. The -K option leaves the build directory and you will see the path to it somewhere at the end of the output when failing. It's a directory in /private/tmp
If you want to run just the specific lit tests from there you need to use: nix-shell -A ldc
For this to work you need to be in the repository directory too. This should get you into an environment/shell equal to the build. In there you can switch to the temp build dir and just rerun the lit tests or whatever.
It might be possible that you get permission problems on the rerun. In this case you need to move the temp build dir somewhere else.

Hope this helps.
I can also offer you to try to fix it for myself but as it is now, I don't have a clue what's going on with this dynamiccompile stuff at all.
You will need to brief me.

The changes you made are mainly in linker-gcc.cpp as far as I remember. Because of the file name I guess it has something to do with gcc. Maybe that is the problem? In the package description for ldc in Nix there is no dependency to gcc at all. It's just clang with llvm.

You can see all dependencies on top of the package description file which can be found in the repository at: pkgs/development/compilers/ldc/default.nix

JohanEngelen · 2018-01-22T20:33:53Z

fyi, linker-gcc.cpp handles all non-windows linking. On macOS, we use the gcc driver for linking (it's just a front for clang).

ThomasMader · 2018-01-26T21:31:52Z

You need to checkout https://github.com/ThomasMader/nixpkgs/tree/dynamiccompile

@Hardcode84 Just realized that I forgot to push the change to use your ldc branch in the package description.
Now it should be fine.

kinke · 2018-04-22T13:18:52Z

Update: current master already makes sure the install_name of shared lib ldc-jit contains @rpath. It also sets the default rpath when linking with -enable-dynamic-compile. It doesn't set an additional @executable_path rpath as this PR though - I think that should be left to the user, as it depends on how he/she is going to deploy the software (e.g., may use @executable_path/../lib or whatever).

OSX jit dylib link fix

fbd535f

Hardcode84 mentioned this pull request Jan 14, 2018

dynamiccompile tests not successful on NixOS #2497

Closed

JohanEngelen reviewed Jan 14, 2018

View reviewed changes

JohanEngelen reviewed Jan 16, 2018

View reviewed changes

Hardcode84 force-pushed the jit_osx_link_fix branch from 70af4fa to 1f3d293 Compare January 17, 2018 15:15

review fixes

94d1264

Hardcode84 force-pushed the jit_osx_link_fix branch from 1f3d293 to 94d1264 Compare January 17, 2018 15:16

Hardcode84 mentioned this pull request Jan 22, 2018

Jit known bugs and possible improvements #2535

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Attempt to fix #2497 #2503

[WIP] Attempt to fix #2497 #2503

Hardcode84 commented Jan 14, 2018

JohanEngelen Jan 14, 2018

JohanEngelen Jan 14, 2018

Hardcode84 Jan 15, 2018

JohanEngelen Jan 14, 2018

JohanEngelen commented Jan 14, 2018

Hardcode84 commented Jan 16, 2018

JohanEngelen Jan 16, 2018

Hardcode84 Jan 17, 2018

JohanEngelen Jan 16, 2018

Hardcode84 Jan 17, 2018

JohanEngelen Jan 16, 2018

Hardcode84 Jan 17, 2018

JohanEngelen Jan 17, 2018

Hardcode84 commented Jan 17, 2018

JohanEngelen commented Jan 17, 2018

ThomasMader commented Jan 20, 2018

Hardcode84 commented Jan 22, 2018

ThomasMader commented Jan 22, 2018

JohanEngelen commented Jan 22, 2018

ThomasMader commented Jan 26, 2018

kinke commented Apr 22, 2018


		void addUniqueArgs(const std::vector<std::string>& arg);

		std::set<std::vector<std::string>> uniqueArgs;

[WIP] Attempt to fix #2497 #2503

Are you sure you want to change the base?

[WIP] Attempt to fix #2497 #2503

Conversation

Hardcode84 commented Jan 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanEngelen commented Jan 14, 2018

Hardcode84 commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hardcode84 commented Jan 17, 2018

JohanEngelen commented Jan 17, 2018

ThomasMader commented Jan 20, 2018

Hardcode84 commented Jan 22, 2018

ThomasMader commented Jan 22, 2018

JohanEngelen commented Jan 22, 2018

ThomasMader commented Jan 26, 2018

kinke commented Apr 22, 2018