Merge pull request #1108 from centricular/fix-optimization-flags-order

Fix optimization flags order

3 files changed, 155 insertions, 32 deletions

diff --git a/mesonbuild/compilers.py b/mesonbuild/compilers.py
index 8f8851f..2534a47 100644
--- a/mesonbuild/compilers.py
+++ b/mesonbuild/compilers.py
@@ -701,8 +701,28 @@ int main () {{
   #endif
   return 0;
 }}'''
-        args = extra_args + self.get_compiler_check_args()
-        return self.compiles(templ.format(hname, symbol, prefix), env, args, dependencies)
+        return self.compiles(templ.format(hname, symbol, prefix), env,
+                             extra_args, dependencies)
+
+    @staticmethod
+    def _override_args(args, override):
+        '''
+        Add @override to @args in such a way that arguments are overriden
+        correctly.
+
+        We want the include directories to be added first (since they are
+        chosen left-to-right) and all other arguments later (since they
+        override previous arguments or add to a list that's chosen
+        right-to-left).
+        '''
+        before_args = []
+        after_args = []
+        for arg in override:
+            if arg.startswith(('-I', '/I')):
+                before_args.append(arg)
+            else:
+                after_args.append(arg)
+        return before_args + args + after_args
 
     def compiles(self, code, env, extra_args=None, dependencies=None):
         if extra_args is None:
@@ -713,9 +733,10 @@ int main () {{
             dependencies = []
         elif not isinstance(dependencies, list):
             dependencies = [dependencies]
+        # Add compile flags needed by dependencies after converting to the
+        # native type of the selected compiler
         cargs = [a for d in dependencies for a in d.get_compile_args()]
-        # Convert flags to the native type of the selected compiler
-        args = self.unix_link_flags_to_native(cargs + extra_args)
+        args = self.unix_link_flags_to_native(cargs)
         # Read c_args/cpp_args/etc from the cross-info file (if needed)
         args += self.get_cross_extra_flags(env, compile=True, link=False)
         # Add CFLAGS/CXXFLAGS/OBJCFLAGS/OBJCXXFLAGS from the env
@@ -723,6 +744,11 @@ int main () {{
         args += env.coredata.external_args[self.language]
         # We only want to compile; not link
         args += self.get_compile_only_args()
+        # Append extra_args to the compiler check args such that it overrides
+        extra_args = self._override_args(self.get_compiler_check_args(), extra_args)
+        extra_args = self.unix_link_flags_to_native(extra_args)
+        # Append both to the compiler args such that they override them
+        args = self._override_args(args, extra_args)
         with self.compile(code, args) as p:
             return p.returncode == 0
 
@@ -736,17 +762,24 @@ int main () {{
             dependencies = []
         elif not isinstance(dependencies, list):
             dependencies = [dependencies]
+        # Add compile and link flags needed by dependencies after converting to
+        # the native type of the selected compiler
         cargs = [a for d in dependencies for a in d.get_compile_args()]
         link_args = [a for d in dependencies for a in d.get_link_args()]
-        # Convert flags to the native type of the selected compiler
-        args = self.unix_link_flags_to_native(cargs + link_args + extra_args)
+        args = self.unix_link_flags_to_native(cargs + link_args)
         # Select a CRT if needed since we're linking
         args += self.get_linker_debug_crt_args()
-        # Read c_args/c_link_args/cpp_args/cpp_link_args/etc from the cross-info file (if needed)
+        # Read c_args/c_link_args/cpp_args/cpp_link_args/etc from the
+        # cross-info file (if needed)
         args += self.get_cross_extra_flags(env, compile=True, link=True)
         # Add LDFLAGS from the env. We assume that the user has ensured these
         # are compiler-specific
         args += env.coredata.external_link_args[self.language]
+        # Append extra_args to the compiler check args such that it overrides
+        extra_args = self._override_args(self.get_compiler_check_args(), extra_args)
+        extra_args = self.unix_link_flags_to_native(extra_args)
+        # Append both to the compiler args such that they override them
+        args = self._override_args(args, extra_args)
         return self.compile(code, args)
 
     def links(self, code, env, extra_args=None, dependencies=None):
@@ -795,7 +828,6 @@ int main(int argc, char **argv) {{
 %s
 int temparray[%d-sizeof(%s)];
 '''
-        args = extra_args + self.get_compiler_check_args()
         if not self.compiles(element_exists_templ.format(prefix, element), env, args, dependencies):
             return -1
         for i in range(1, 1024):
@@ -844,7 +876,6 @@ struct tmp {
 
 int testarray[%d-offsetof(struct tmp, target)];
 '''
-        args = extra_args + self.get_compiler_check_args()
         if not self.compiles(type_exists_templ.format(typename), env, args, dependencies):
             return -1
         for i in range(1, 1024):
@@ -980,14 +1011,14 @@ int main(int argc, char **argv) {
             head, main = self._no_prototype_templ()
         templ = head + stubs_fail + main
 
-        args = extra_args + self.get_compiler_check_args()
-        if self.links(templ.format(prefix, funcname), env, args, dependencies):
+        if self.links(templ.format(prefix, funcname), env, extra_args, dependencies):
             return True
         # Some functions like alloca() are defined as compiler built-ins which
         # are inlined by the compiler, so test for that instead. Built-ins are
         # special functions that ignore all includes and defines, so we just
         # directly try to link via main().
-        return self.links('int main() {{ {0}; }}'.format('__builtin_' + funcname), env, args, dependencies)
+        return self.links('int main() {{ {0}; }}'.format('__builtin_' + funcname),
+                          env, extra_args, dependencies)
 
     def has_members(self, typename, membernames, prefix, env, extra_args=None, dependencies=None):
         if extra_args is None:
@@ -1071,8 +1102,8 @@ class CPPCompiler(CCompiler):
 #include <{0}>
 using {1};
 int main () {{ return 0; }}'''
-        args = extra_args + self.get_compiler_check_args()
-        return self.compiles(templ.format(hname, symbol, prefix), env, args, dependencies)
+        return self.compiles(templ.format(hname, symbol, prefix), env,
+                             extra_args, dependencies)
 
 class ObjCCompiler(CCompiler):
     def __init__(self, exelist, version, is_cross, exe_wrap):
diff --git a/run_unittests.py b/run_unittests.py
index 8b1f13f..03ce0df 100755
--- a/run_unittests.py
+++ b/run_unittests.py
@@ -24,9 +24,10 @@ from mesonbuild.dependencies import PkgConfigDependency, Qt5Dependency
 
 def get_soname(fname):
     # HACK, fix to not use shell.
-    raw_out = subprocess.check_output(['readelf', '-a', fname])
-    pattern = re.compile(b'soname: \[(.*?)\]')
-    for line in raw_out.split(b'\n'):
+    raw_out = subprocess.check_output(['readelf', '-a', fname],
+                                      universal_newlines=True)
+    pattern = re.compile('soname: \[(.*?)\]')
+    for line in raw_out.split('\n'):
         m = pattern.search(line)
         if m is not None:
             return m.group(1)
@@ -94,27 +95,55 @@ class LinuxlikeTests(unittest.TestCase):
         with open(os.path.join(self.builddir, 'meson-logs', 'meson-log.txt')) as f:
             return f.readlines()
 
+    def get_meson_log_compiler_checks(self):
+        '''
+        Fetch a list command-lines run by meson for compiler checks.
+        Each command-line is returned as a list of arguments.
+        '''
+        log = self.get_meson_log()
+        prefix = 'Command line:'
+        cmds = [l[len(prefix):].split() for l in log if l.startswith(prefix)]
+        return cmds
+
     def introspect(self, arg):
-        out = subprocess.check_output(self.mintro_command + [arg, self.builddir])
-        return json.loads(out.decode('utf-8'))
+        out = subprocess.check_output(self.mintro_command + [arg, self.builddir],
+                                      universal_newlines=True)
+        return json.loads(out)
 
     def test_basic_soname(self):
+        '''
+        Test that the soname is set correctly for shared libraries. This can't
+        be an ordinary test case because we need to run `readelf` and actually
+        check the soname.
+        https://github.com/mesonbuild/meson/issues/785
+        '''
         testdir = os.path.join(self.common_test_dir, '4 shared')
         self.init(testdir)
         self.build()
         lib1 = os.path.join(self.builddir, 'libmylib.so')
         soname = get_soname(lib1)
-        self.assertEqual(soname, b'libmylib.so')
+        self.assertEqual(soname, 'libmylib.so')
 
     def test_custom_soname(self):
+        '''
+        Test that the soname is set correctly for shared libraries when
+        a custom prefix and/or suffix is used. This can't be an ordinary test
+        case because we need to run `readelf` and actually check the soname.
+        https://github.com/mesonbuild/meson/issues/785
+        '''
         testdir = os.path.join(self.common_test_dir, '27 library versions')
         self.init(testdir)
         self.build()
         lib1 = os.path.join(self.builddir, 'prefixsomelib.suffix')
         soname = get_soname(lib1)
-        self.assertEqual(soname, b'prefixsomelib.suffix')
+        self.assertEqual(soname, 'prefixsomelib.suffix')
 
     def test_pic(self):
+        '''
+        Test that -fPIC is correctly added to static libraries when b_staticpic
+        is true and not when it is false. This can't be an ordinary test case
+        because we need to inspect the compiler database.
+        '''
         testdir = os.path.join(self.common_test_dir, '3 static')
         self.init(testdir)
         compdb = self.get_compdb()
@@ -130,6 +159,12 @@ class LinuxlikeTests(unittest.TestCase):
         self.assertTrue('-fPIC' not in compdb[0]['command'])
 
     def test_pkgconfig_gen(self):
+        '''
+        Test that generated pkg-config files can be found and have the correct
+        version and link args. This can't be an ordinary test case because we
+        need to run pkg-config outside of a Meson build file.
+        https://github.com/mesonbuild/meson/issues/889
+        '''
         testdir = os.path.join(self.common_test_dir, '51 pkgconfig-gen')
         self.init(testdir)
         env = FakeEnvironment()
@@ -141,6 +176,12 @@ class LinuxlikeTests(unittest.TestCase):
         self.assertTrue('-lfoo' in simple_dep.get_link_args())
 
     def test_vala_c_warnings(self):
+        '''
+        Test that no warnings are emitted for C code generated by Vala. This
+        can't be an ordinary test case because we need to inspect the compiler
+        database.
+        https://github.com/mesonbuild/meson/issues/864
+        '''
         testdir = os.path.join(self.vala_test_dir, '5 target glib')
         self.init(testdir)
         compdb = self.get_compdb()
@@ -168,6 +209,12 @@ class LinuxlikeTests(unittest.TestCase):
         self.assertTrue('-Werror' in c_command)
 
     def test_static_compile_order(self):
+        '''
+        Test that the order of files in a compiler command-line while compiling
+        and linking statically is deterministic. This can't be an ordinary test
+        case because we need to inspect the compiler database.
+        https://github.com/mesonbuild/meson/pull/951
+        '''
         testdir = os.path.join(self.common_test_dir, '5 linkstatic')
         self.init(testdir)
         compdb = self.get_compdb()
@@ -179,6 +226,10 @@ class LinuxlikeTests(unittest.TestCase):
         # FIXME: We don't have access to the linker command
 
     def test_install_introspection(self):
+        '''
+        Tests that the Meson introspection API exposes install filenames correctly
+        https://github.com/mesonbuild/meson/issues/829
+        '''
         testdir = os.path.join(self.common_test_dir, '8 install')
         self.init(testdir)
         intro = self.introspect('--targets')
@@ -188,11 +239,19 @@ class LinuxlikeTests(unittest.TestCase):
         self.assertEqual(intro[1]['install_filename'], '/usr/local/bin/prog')
 
     def test_run_target_files_path(self):
+        '''
+        Test that run_targets are run from the correct directory
+        https://github.com/mesonbuild/meson/issues/957
+        '''
         testdir = os.path.join(self.common_test_dir, '58 run target')
         self.init(testdir)
         self.run_target('check_exists')
 
     def test_qt5dependency_qmake_detection(self):
+        '''
+        Test that qt5 detection with qmake works. This can't be an ordinary
+        test case because it involves setting the environment.
+        '''
         # Verify that qmake is for Qt5
         if not shutil.which('qmake-qt5'):
             if not shutil.which('qmake'):
@@ -215,8 +274,9 @@ class LinuxlikeTests(unittest.TestCase):
         self.assertTrue(msg in mesonlog or msg2 in mesonlog)
 
     def get_soname(self, fname):
-        output = subprocess.check_output(['readelf', '-a', fname])
-        for line in output.decode('utf-8', errors='ignore').split('\n'):
+        output = subprocess.check_output(['readelf', '-a', fname],
+                                         universal_newlines=True)
+        for line in output.split('\n'):
             if 'SONAME' in line:
                 return line.split('[')[1].split(']')[0]
         raise RuntimeError('Readelf gave no SONAME.')
@@ -262,5 +322,23 @@ class LinuxlikeTests(unittest.TestCase):
         self.assertEqual(self.get_soname(bothset), 'libbothset.so.1.2.3')
         self.assertEqual(len(glob(bothset[:-3] + '*')), 3)
 
+    def test_compiler_check_flags_order(self):
+        '''
+        Test that compiler check flags override all other flags. This can't be
+        an ordinary test case because it needs the environment to be set.
+        '''
+        Oflag = '-O3'
+        os.environ['CFLAGS'] = os.environ['CXXFLAGS'] = Oflag
+        testdir = os.path.join(self.common_test_dir, '43 has function')
+        self.init(testdir)
+        cmds = self.get_meson_log_compiler_checks()
+        for cmd in cmds:
+            # Verify that -I flags from the `args` kwarg are first
+            # This is set in the '43 has function' test case
+            self.assertEqual(cmd[2], '-I/tmp')
+            # Verify that -O3 set via the environment is overriden by -O0
+            Oargs = [arg for arg in cmd if arg.startswith('-O')]
+            self.assertEqual(Oargs, [Oflag, '-O0'])
+
 if __name__ == '__main__':
     unittest.main()
diff --git a/test cases/common/43 has function/meson.build b/test cases/common/43 has function/meson.build
index 61f96e1..e0d3344 100644
--- a/test cases/common/43 has function/meson.build
+++ b/test cases/common/43 has function/meson.build
@@ -1,9 +1,12 @@
 project('has function', 'c', 'cpp')
 
+# This is used in the `test_compiler_check_flags_order` unit test
+unit_test_args = '-I/tmp'
 compilers = [meson.get_compiler('c'), meson.get_compiler('cpp')]
 
 foreach cc : compilers
-  if not cc.has_function('printf', prefix : '#include<stdio.h>')
+  if not cc.has_function('printf', prefix : '#include<stdio.h>',
+                         args : unit_test_args)
     error('"printf" function not found (should always exist).')
   endif
 
@@ -13,12 +16,16 @@ foreach cc : compilers
   # On MSVC fprintf is defined as an inline function in the header, so it cannot
   # be found without the include.
   if cc.get_id() != 'msvc'
-    assert(cc.has_function('fprintf'), '"fprintf" function not found without include (on !msvc).')
+    assert(cc.has_function('fprintf', args : unit_test_args),
+           '"fprintf" function not found without include (on !msvc).')
   else
-    assert(cc.has_function('fprintf', prefix : '#include <stdio.h>'), '"fprintf" function not found with include (on msvc).')
+    assert(cc.has_function('fprintf', prefix : '#include <stdio.h>',
+                           args : unit_test_args),
+           '"fprintf" function not found with include (on msvc).')
   endif
 
-  if cc.has_function('hfkerhisadf', prefix : '#include<stdio.h>')
+  if cc.has_function('hfkerhisadf', prefix : '#include<stdio.h>',
+                     args : unit_test_args)
     error('Found non-existent function "hfkerhisadf".')
   endif
 
@@ -28,16 +35,23 @@ foreach cc : compilers
   # implemented in glibc it's probably not implemented in any other 'slimmer'
   # C library variants either, so the check should be safe either way hopefully.
   if host_machine.system() == 'linux' and cc.get_id() == 'gcc'
-    assert (cc.has_function('poll', prefix : '#include <poll.h>'), 'couldn\'t detect "poll" when defined by a header')
-    assert (not cc.has_function('lchmod', prefix : '''#include <sys/stat.h>
-                                                      #include <unistd.h>'''), '"lchmod" check should have failed')
+    assert (cc.has_function('poll', prefix : '#include <poll.h>',
+                            args : unit_test_args),
+            'couldn\'t detect "poll" when defined by a header')
+    lchmod_prefix = '#include <sys/stat.h>\n#include <unistd.h>'
+    assert (not cc.has_function('lchmod', prefix : lchmod_prefix,
+                                args : unit_test_args),
+            '"lchmod" check should have failed')
   endif
 
   # For some functions one needs to define _GNU_SOURCE before including the
   # right headers to get them picked up. Make sure we can detect these functions
   # as well without any prefix
-  if cc.has_header_symbol('sys/socket.h', 'recvmmsg', prefix : '#define _GNU_SOURCE')
+  if cc.has_header_symbol('sys/socket.h', 'recvmmsg',
+                          prefix : '#define _GNU_SOURCE',
+                          args : unit_test_args)
     # We assume that if recvmmsg exists sendmmsg does too
-    assert (cc.has_function('sendmmsg'), 'Failed to detect function "sendmmsg" (should always exist).')
+    assert (cc.has_function('sendmmsg', args : unit_test_args),
+            'Failed to detect function "sendmmsg" (should always exist).')
   endif
 endforeach