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

Asmlink.reset should also clean lib_ccobjs/lib_ccopts #1624

Merged
merged 1 commit into from Feb 21, 2018

Conversation

Projects
None yet
2 participants
@rixed
Copy link
Contributor

rixed commented Feb 21, 2018

As stated in ticket 7738 (https://caml.inria.fr/mantis/view.php?id=7738):

When using the compiler-libs in a program and trying to compile several source codes I noticed some options to gcc accumulated from one compilation to the next, despite I've tried to cleaned as many global variables as I could, including via Asmlink.reset().

Problem seems to be that the aforementioned reset function does not clean lib_ccobjs or lib_ccopts.
A quick test suggest that cleaning those does fix my issue without introducing any bug I could notice.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 21, 2018

The change is correct, but I wonder if it is sufficient. I am worried in particular by the following statements in both link_shared and link functions:

    Clflags.ccobjs := !Clflags.ccobjs @ !lib_ccobjs;
    Clflags.all_ccopts := !lib_ccopts @ !Clflags.all_ccopts;
                                                 (* put user's opts first *)

Right now the reset function does not revert this change (and I see no non-hackish way to do it), so I suspect that you would not get reentrant library uses if you used them. I see no better way to solve this than removing these horrible mutations and transporting the ccobjs and all_ccopts as additional parameters to the callees that access them -- call_linker in particular, but maybe others.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 21, 2018

(Also, please provide a Changes entry in the "Internal/compiler-libs changes" category, with both the Mantis issue number (MPR) and Github PR number (GPR).)

@rixed

This comment has been minimized.

Copy link
Contributor Author

rixed commented Feb 21, 2018

Clflags.ccobjs and other values in Clflags are publicly accessible and therefore can be (re)set in between several compilations as required. (There are many other such global parameters actually that has to be reset by hand).

@gasche

gasche approved these changes Feb 21, 2018

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 21, 2018

Ok. I still believe that non-driving modules should not modify Clflags values, only access them, but the current change is reasonably scoped for a pull request, and your explanation clarifies that the Clflags issue can be worked around.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 21, 2018

This is good to merge if the CI passes and you write a Changes entry.

@rixed

This comment has been minimized.

Copy link
Contributor Author

rixed commented Feb 21, 2018

Thank you, going to write that changelog entry now.

@rixed rixed force-pushed the rixed:trunk branch from a0e2ddb to 75042a2 Feb 21, 2018

@gasche gasche merged commit a7ca6cc into ocaml:trunk Feb 21, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.