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 linking problem in sac_from_c, caused by ld being a one-pass linker #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bernecky
Copy link

@bernecky bernecky commented Apr 8, 2021

No description provided.

@sbscholz
Copy link
Member

sbscholz commented Apr 8, 2021

AFAIK, the problem is a bit deeper. Will the fix you propose solves your problem, it creates the dual problem for systems where linking is done the other way around. If this is indeed the case, we need to find a way to detect what the system needs; this should be possible within CMAKE...

@bernecky
Copy link
Author

bernecky commented Apr 8, 2021

Re Bodo's comment, above: Can you cite any single-pass linker/loaders that work in the opposite
direction (i.e., right-to-left, rather than left-to-right as gcc's ld does)? If not, we may be okay.

As the code stands in sac_from_c/CMakeLists.txt now, the offending argument sequence was
hand-coded, in an ADD_CUSTOM_COMMAND, so unless you are able to generate most or all of that sequence within a .cmake file, that won't solve any future problems in that area. Perhaps it's trivial, but I don't know: this is my first rodeo.

@ashinkarov
Copy link
Collaborator

@sbscholz , Cmake doesn't really help here, as -I and -L flags (and -lmod1 -lmod2) are generated by sac4c dynamically with some assumptions in mind. Cmake would have worked very well if we were able to know all the needed flags statically, i.e. at the time we run cmake. In this case we could have used ADD_EXECUTABLE and append target properties. In principle this is possible, as we can precompute the output that sac4c generates, but we'd have to hardcode some assumptions that might change in the future, so I suggest not to do this.

Alternatively, and this is what we are doing right now, we have to generate the command that calls the C compiler in the right way. There is always a risk that this would be incorrect for some compiler. While it should be possible to adjust CMAKE_C_COMPILE_OBJECT and friends with some generator expression magic, this is non-trivial, and sac4c would have to become more specific in its output (instead of generating a lump of flag salad, it would have to produce them in cmake-friendly way).

But maybe, before doing all this, I think that Bob's solution is actually reasonable. A single-pass linker is the simplest kind that we can find in the wild, so if it accepts our commands, any other linker should do so as well. Do you have a specific example of a linker that would fail?

@bernecky
Copy link
Author

bernecky commented Apr 9, 2021

Are the -lmod1 and -lmod2 flags blindly introduced in order to support some maybe-run-time codes? Just curious.
It seems a bit sloppy/lazy to me...

WRT -l stuff, I read somewhere that CMAKE does have the capability to sort the library trees into a correct order
for single-pass linking, but that is just a rumor. I have no idea what, if anything, sac4c does about -l/-L ordering.

@ashinkarov
Copy link
Collaborator

@bernecky , the -lmod1 are all those modules that the sac sources depend on. I.e. in case of sac_from_c, these are going to be all the modules that Sum.sac depends on: -lSumMod, -lArrayMod, -lArrayTransformMod, etc.

As for cmake, yes, it knows how to build the targets correctly and it would do the right thing. The catch is that you have to give dependencies statically (when you run cmake). In case of sac_from_c, we are dealing with dynamic dependencies: we have to compile Sum.sac before we can call sac4c to figure out the flags that are needed for summarize.

Once again, it is possible to work around this problem, but the solution is non-trivial. Therefore, maybe in this particular case of a single binary, what you are proposing is ok.

@bernecky
Copy link
Author

Hi, Artem. Your "generated by sac4c dynamically with some assumptions..." threw me. I assumed
that the assumptions were somehow wired into sac4c. Your more recent comment clarifies that.

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.

3 participants