Skip to content

Commit

Permalink
compilers: Try harder to dedup builtin libs
Browse files Browse the repository at this point in the history
-ldl, -lm, -lpthread should always be de-duplicated, no matter what.

Closes #2150
  • Loading branch information
nirbheek committed Mar 12, 2019
1 parent 9409155 commit 8b06553
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 1 deletion.
5 changes: 4 additions & 1 deletion mesonbuild/compilers/compilers.py
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,9 @@ class CompilerArgs(list):
# Only UNIX shared libraries require this. Others have a fixed extension.
dedup1_regex = re.compile(r'([\/\\]|\A)lib.*\.so(\.[0-9]+)?(\.[0-9]+)?(\.[0-9]+)?$')
dedup1_args = ('-c', '-S', '-E', '-pipe', '-pthread')
# In generate_link() we add external libs without de-dup, but we must
# *always* de-dup these because they're special arguments to the linker
always_dedup_args = ('-ldl', '-lm', '-lpthread')
compiler = None

def _check_args(self, args):
Expand Down Expand Up @@ -793,7 +796,7 @@ def extend_preserving_lflags(self, iterable):
normal_flags = []
lflags = []
for i in iterable:
if i.startswith('-l') or i.startswith('-L'):
if i not in self.always_dedup_args and (i.startswith('-l') or i.startswith('-L')):
lflags.append(i)
else:
normal_flags.append(i)
Expand Down
13 changes: 13 additions & 0 deletions run_unittests.py
Original file line number Diff line number Diff line change
Expand Up @@ -5039,6 +5039,19 @@ def test_ldflag_dedup(self):
max_count = max(max_count, line.count(search_term))
self.assertEqual(max_count, 1, 'Export dynamic incorrectly deduplicated.')

def test_compiler_libs_static_dedup(self):
testdir = os.path.join(self.unit_test_dir, '55 dedup compiler libs')
self.init(testdir)
build_ninja = os.path.join(self.builddir, 'build.ninja')
with open(build_ninja, 'r', encoding='utf-8') as f:
lines = f.readlines()
for lib in ('-ldl', '-lm'):
for line in lines:
if lib not in line:
continue
# Assert that
self.assertEqual(len(line.split(lib)), 2, msg=(lib, line))


def should_run_cross_arm_tests():
return shutil.which('arm-linux-gnueabihf-gcc') and not platform.machine().lower().startswith('arm')
Expand Down

4 comments on commit 8b06553

@bruce-richardson
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks too specific to those libraries. If I go and add a dependency on e.g. libnuma or libpcap to the base DPDK EAL library, for instance, I will hit exactly the same problems.

Looking at the build.ninja file generated, it appears that all the library flags are being enclosed in "--start-group" and "--end-group" flags, which means that all "-l" flags can be deduplicated as they won't be needed more than once.

@nirbheek
Copy link
Member Author

@nirbheek nirbheek commented on 8b06553 Mar 13, 2019

Choose a reason for hiding this comment

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

This looks too specific to those libraries. If I go and add a dependency on e.g. libnuma or libpcap to the base DPDK EAL library, for instance, I will hit exactly the same problems.

It is indeed too specific to these libraries, but this is a fix that can go into the stable release since it's low-risk.

Generally, external dependencies are resolved to the full path to the .a, .so, .dylib, or .lib, so this shouldn't be happening for other libraries. We made it so that -L and -l pairs are added as-is for a 'conservative' fix to the problem of always picking libraries from the correct -L path. I think changing that code will definitely lead to regressions, so it's something for the next release if someone wants to work on it.

@bruce-richardson
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, understood on the risk aspect. Can you perhaps also include "-lexecinfo" into the list, as it's a frequently needed library on freebsd.

@nirbheek
Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, no problem.

Please sign in to comment.