Skip to content

Map user-specified system libraries to correct variants #14355

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

Merged
merged 1 commit into from
Jun 4, 2021
Merged

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jun 3, 2021

Normally users don't specify system libraries such as
libc or compiler-rt on the command line. However, when
they do, it makes sense map them to correct variant.

For example, linking with -pthread + -lc will now
end up including libc-mt.a rather than libc.a.

Fixes: #14341

@sbc100 sbc100 requested a review from kripken June 3, 2021 00:52
@sbc100 sbc100 force-pushed the use_library_map branch from b87f6d4 to 6aaf439 Compare June 3, 2021 00:54
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Testcase?

@sbc100
Copy link
Collaborator Author

sbc100 commented Jun 3, 2021

Added test case.

@sbc100 sbc100 force-pushed the use_library_map branch 2 times, most recently from dfee207 to b26eb22 Compare June 3, 2021 22:27
@sbc100
Copy link
Collaborator Author

sbc100 commented Jun 3, 2021

Found that I could do this without adding a new pass over the libraries.. much cleaner.

@sbc100 sbc100 requested a review from kripken June 4, 2021 00:35
Normally users don't specify system libraries such as
libc or compiler-rt on the command line.  However, when
they do it makes sense map them to correct variant.

For example, linking with `-pthread` + `-lc` will now
end up including `libc-mt.a` rather than `libc.a`.

Fixes: #14341
@sbc100 sbc100 force-pushed the use_library_map branch from b26eb22 to 92fc305 Compare June 4, 2021 03:19

def test_standard_library_mapping(self):
# Test the `-l` flags on the command line get mapped the correct libraries variant
self.run_process([EMBUILDER, 'build', 'libc-mt', 'libcompiler_rt-mt', 'libdlmalloc-mt'])
Copy link
Member

Choose a reason for hiding this comment

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

Why is the embuilder line needed? (auto library building should happen on line 10639?)

lgtm without line 10636

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Automatic building currently only works for libraries that are added automatically in system_libs.py:calculate().

Libraries specified on the command line are not currently auto-built. This is just how its always been as far as I cant tell. We could change that but its what this change is about.

Also, -nostdlib essentially bypasses system_libs.py:calculate() and doesn't do any auto-adding or auto-building of libraries.

This PR is an incremental improvement. We could integrating -l flags with the auto-building system but that would be bigger/separate change.

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.

Should we map libraries specified on the command line to the current variant? -lgl to the required variant
2 participants