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

Fix issue 15574 - wrong order of link arguments #8172

Merged
merged 2 commits into from
Apr 17, 2018

Conversation

rtbo
Copy link
Contributor

@rtbo rtbo commented Apr 15, 2018

This essentially places *.a arguments before the -l and -L switches in
the linker command line.

This allows to link applications that link against a static archive *.a
that depends on -l lib switches.
Such dependency scheme is typical of the following situation:

  • have a D package that contains binding to a C library.
  • this D package is itself built as a static library (libdpackage.a).
  • the C library is built in the same build process as a static library
    (pathtoClib/libclib.a)
  • dmd is invoked in the following way:
  $ dmd ... libdpackage.a -L-LpathtoClib -L-lclib

Currently dmd reorders arguments and places *.a after -l linker switches
which causes link errors if the -l points to a static library.

This is especially useful with Dub, which can be configured to probe for
the C library, and build it as a static lib if not found system-wide.
In both cases, the C library is provided with "libs" json entry.

So this is not about keeping relative order of linker arguments, it is
about providing an order of linker arguments that is more sound than
the current one.

This essentially places *.a arguments before the -l and -L switches in
the linker command line.

This allows to link applications that link against a static archive *.a
that depends on -l lib switches.
Such dependency scheme is typical of the following situation:
  - have a D package that contains binding to a C library.
  - this D package is itself built as a static library (libdpackage.a).
  - the C library is built in the same build process as a static library
    (pathtoClib/libclib.a)
  - dmd is invoked in the following way:

  $ dmd ... libdpackage.a -L-LpathtoClib -L-lclib

Currently dmd reorders arguments and places *.a after -l linker switches
which causes link errors if the -l points to a static library.

This is especially useful with Dub, which can be configured to probe for
the C library, and build it as a static lib if not found system-wide.
In both cases, the C library is provided with "libs" json entry.

So this is not about keeping relative order of linker arguments, it is
about providing an order of linker arguments that is more sound than
the current one.
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @rtbo! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
15574 blocker wrong order of linker arguments

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

To target stable perform these two steps:

  1. Rebase your branch to upstream/stable:
git rebase --onto upstream/stable upstream/master
  1. Change the base branch of your PR to stable

Testing this PR locally

If 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#8172"

@rtbo
Copy link
Contributor Author

rtbo commented Apr 15, 2018

I have here a simple test case:

git clone https://github.com/rtbo/dmd_link_args_order
cd dmd_link_args_order
make app

Fails to link with dmd from master, links with this PR.
Not sure if and how to integrate this test in the dmd test suite.

@wilzbach
Copy link
Member

@rtbo have a look at the existing bash scripts in the testsuite for examples on how this can be added to testsuite.

@rtbo rtbo force-pushed the linker_arg_order branch from e5fcaaf to 7bcdd6e Compare April 16, 2018 00:36
@RazvanN7
Copy link
Contributor

The documentation failure is unrelated to this PR.


${DMD} -m${MODEL} -lib -of${D_LIB} ${D_FILE}

${DMD} -m${MODEL} -of${APP} ${APP_FILE} -I${TEST_DIR} ${D_LIB} -L-L${TEST_DIR} -L-lcsquare
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also run the executable to make sure that everything is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. (and squashed)

@rtbo rtbo force-pushed the linker_arg_order branch from 7bcdd6e to 10c8fa5 Compare April 16, 2018 21:26

${DMD} -m${MODEL} -of${APP} ${APP_FILE} -I${TEST_DIR} ${D_LIB} -L-L${TEST_DIR} -L-lcsquare

${APP}
Copy link
Contributor

@RazvanN7 RazvanN7 Apr 17, 2018

Choose a reason for hiding this comment

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

It would be best if you could check that the application actually runs correctly i.e. it finished with exit code 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't the test fail if a command returns non-zero?

Copy link
Contributor

@RazvanN7 RazvanN7 Apr 17, 2018

Choose a reason for hiding this comment

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

you are correct. sorry for the confusion

@RazvanN7
Copy link
Contributor

@rtbo I added the merge label on this PR. Thank you for your contribution

@dlang-bot dlang-bot merged commit 6371942 into dlang:master Apr 17, 2018
@rtbo rtbo deleted the linker_arg_order branch April 18, 2018 21:27
@CyberShadow
Copy link
Member

@rtbo This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=19243

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants