From d8b9b12adbd2fa04f40ea2823929acc41f0a8003 Mon Sep 17 00:00:00 2001 From: Nirbheek Chauhan Date: Thu, 14 Jul 2016 17:49:40 +0530 Subject: [PATCH 1/3] Ninja now supports backslash in command args, so we can too At the same time, this also adds a bunch of tests that document and keep track of how we expect quoting to pass through via Ninja to the compiler. We need at least Ninja 1.6.0 for this. This fixes https://github.com/mesonbuild/meson/issues/489 --- mesonbuild/build.py | 24 ---------------- .../asm output/meson.build | 2 ++ .../comparer-end-notstring.c | 20 +++++++++++++ .../115 spaces backslash/comparer-end.c | 16 +++++++++++ .../common/115 spaces backslash/comparer.c | 16 +++++++++++ .../115 spaces backslash/include/comparer.h | 4 +++ .../common/115 spaces backslash/meson.build | 28 +++++++++++++++++++ test cases/failing/24 backslash/comparer.c | 10 ------- test cases/failing/24 backslash/meson.build | 3 -- 9 files changed, 86 insertions(+), 37 deletions(-) create mode 100644 test cases/common/115 spaces backslash/asm output/meson.build create mode 100644 test cases/common/115 spaces backslash/comparer-end-notstring.c create mode 100644 test cases/common/115 spaces backslash/comparer-end.c create mode 100644 test cases/common/115 spaces backslash/comparer.c create mode 100644 test cases/common/115 spaces backslash/include/comparer.h create mode 100644 test cases/common/115 spaces backslash/meson.build delete mode 100644 test cases/failing/24 backslash/comparer.c delete mode 100644 test cases/failing/24 backslash/meson.build diff --git a/mesonbuild/build.py b/mesonbuild/build.py index b610bb8d9..6a4f37596 100644 --- a/mesonbuild/build.py +++ b/mesonbuild/build.py @@ -50,28 +50,6 @@ known_shlib_kwargs.update({'version' : True, 'name_suffix' : True, 'vs_module_defs' : True}) -backslash_explanation = \ -'''Compiler arguments have a backslash "\\" character. This is unfortunately not -permitted. The reason for this is that backslash is a shell quoting character -that behaves differently across different systems. Because of this is it not -possible to make it work reliably across all the platforms Meson needs to -support. - -There are several different ways of working around this issue. Most of the time -you are using this to provide a -D define to your compiler. Try instead to -create a config.h file and put all of your definitions in it using -configure_file(). - -Another approach is to move the backslashes into the source and have the other -bits in the def. So you would have an arg -DPLAIN_TEXT="foo" and then in your -C sources something like this: - -const char *fulltext = "\\\\" PLAIN_TEXT; - -We are fully aware that these are not really usable or pleasant ways to do -this but it's the best we can do given the way shell quoting works. -''' - def sources_are_suffix(sources, suffix): for source in sources: if source.endswith('.' + suffix): @@ -606,8 +584,6 @@ class BuildTarget(): for a in args: if not isinstance(a, (str, File)): raise InvalidArguments('A non-string passed to compiler args.') - if isinstance(a, str) and '\\' in a: - raise InvalidArguments(backslash_explanation) if language in self.extra_args: self.extra_args[language] += args else: diff --git a/test cases/common/115 spaces backslash/asm output/meson.build b/test cases/common/115 spaces backslash/asm output/meson.build new file mode 100644 index 000000000..b5f13f5ca --- /dev/null +++ b/test cases/common/115 spaces backslash/asm output/meson.build @@ -0,0 +1,2 @@ +configure_file(output : 'blank.txt', configuration : configuration_data()) + diff --git a/test cases/common/115 spaces backslash/comparer-end-notstring.c b/test cases/common/115 spaces backslash/comparer-end-notstring.c new file mode 100644 index 000000000..65bf8bc55 --- /dev/null +++ b/test cases/common/115 spaces backslash/comparer-end-notstring.c @@ -0,0 +1,20 @@ +#include "comparer.h" + +#ifndef COMPARER_INCLUDED +#error "comparer.h not included" +#endif + +/* This converts foo\\\\bar\\\\ to "foo\\bar\\" (string literal) */ +#define Q(x) #x +#define QUOTE(x) Q(x) + +#define COMPARE_WITH "foo\\bar\\" /* This is the literal `foo\bar\` */ + +int main(int argc, char **argv) { + if(strcmp(QUOTE(DEF_WITH_BACKSLASH), COMPARE_WITH)) { + printf("Arg string is quoted incorrectly: %s instead of %s\n", + QUOTE(DEF_WITH_BACKSLASH), COMPARE_WITH); + return 1; + } + return 0; +} diff --git a/test cases/common/115 spaces backslash/comparer-end.c b/test cases/common/115 spaces backslash/comparer-end.c new file mode 100644 index 000000000..fef25a545 --- /dev/null +++ b/test cases/common/115 spaces backslash/comparer-end.c @@ -0,0 +1,16 @@ +#include "comparer.h" + +#ifndef COMPARER_INCLUDED +#error "comparer.h not included" +#endif + +#define COMPARE_WITH "foo\\bar\\" /* This is `foo\bar\` */ + +int main (int argc, char **argv) { + if (strcmp (DEF_WITH_BACKSLASH, COMPARE_WITH)) { + printf ("Arg string is quoted incorrectly: %s vs %s\n", + DEF_WITH_BACKSLASH, COMPARE_WITH); + return 1; + } + return 0; +} diff --git a/test cases/common/115 spaces backslash/comparer.c b/test cases/common/115 spaces backslash/comparer.c new file mode 100644 index 000000000..937cb47f0 --- /dev/null +++ b/test cases/common/115 spaces backslash/comparer.c @@ -0,0 +1,16 @@ +#include "comparer.h" + +#ifndef COMPARER_INCLUDED +#error "comparer.h not included" +#endif + +#define COMPARE_WITH "foo\\bar" /* This is the literal `foo\bar` */ + +int main (int argc, char **argv) { + if (strcmp (DEF_WITH_BACKSLASH, COMPARE_WITH)) { + printf ("Arg string is quoted incorrectly: %s instead of %s\n", + DEF_WITH_BACKSLASH, COMPARE_WITH); + return 1; + } + return 0; +} diff --git a/test cases/common/115 spaces backslash/include/comparer.h b/test cases/common/115 spaces backslash/include/comparer.h new file mode 100644 index 000000000..624d96c92 --- /dev/null +++ b/test cases/common/115 spaces backslash/include/comparer.h @@ -0,0 +1,4 @@ +#include +#include + +#define COMPARER_INCLUDED diff --git a/test cases/common/115 spaces backslash/meson.build b/test cases/common/115 spaces backslash/meson.build new file mode 100644 index 000000000..bf614e8fc --- /dev/null +++ b/test cases/common/115 spaces backslash/meson.build @@ -0,0 +1,28 @@ +project('comparer', 'c') + +# Added manually as a c_arg to test handling of include paths with backslashes +# and spaces. This is especially useful on Windows in vcxproj files since it +# stores include directories in a separate element that has its own +# context-specific escaping/quoting. +include_dir = meson.current_source_dir() + '/include' +default_c_args = ['-I' + include_dir] + +if meson.get_compiler('c').get_id() == 'msvc' + default_c_args += ['/Faasm output\\'] + # Hack to create the 'asm output' directory in the builddir + subdir('asm output') +endif + +# Path can contain \. Here we're sending `"foo\bar"`. +test('backslash quoting', + executable('comparer', 'comparer.c', + c_args : default_c_args + ['-DDEF_WITH_BACKSLASH="foo\\bar"'])) +# Path can end in \ without any special quoting. Here we send `"foo\bar\"`. +test('backslash end quoting', + executable('comparer-end', 'comparer-end.c', + c_args : default_c_args + ['-DDEF_WITH_BACKSLASH="foo\\bar\\"'])) +# Path can (really) end in \ if we're not passing a string literal without any +# special quoting. Here we're sending `foo\bar\`. +test('backslash end quoting when not a string literal', + executable('comparer-end-notstring', 'comparer-end-notstring.c', + c_args : default_c_args + ['-DDEF_WITH_BACKSLASH=foo\\bar\\'])) diff --git a/test cases/failing/24 backslash/comparer.c b/test cases/failing/24 backslash/comparer.c deleted file mode 100644 index f562f5e37..000000000 --- a/test cases/failing/24 backslash/comparer.c +++ /dev/null @@ -1,10 +0,0 @@ -#include -#include - -int main(int argc, char **argv) { - if(strcmp(DEF_WITH_BACKSLASH, "foo\\bar")) { - printf("Arg string is quoted incorrectly: %s\n", DEF_WITH_BACKSLASH); - return 1; - } - return 0; -} diff --git a/test cases/failing/24 backslash/meson.build b/test cases/failing/24 backslash/meson.build deleted file mode 100644 index dba891eea..000000000 --- a/test cases/failing/24 backslash/meson.build +++ /dev/null @@ -1,3 +0,0 @@ -project('comparer', 'c') - -test('backslash quoting', executable('comparer', 'comparer.c', c_args : '-DDEF_WITH_BACKSLASH="foo\\bar"')) From 2d05008956dedbfb4b6c8b690c4f025451aea587 Mon Sep 17 00:00:00 2001 From: Nirbheek Chauhan Date: Fri, 29 Jul 2016 18:25:43 +0530 Subject: [PATCH 2/3] vs: Fix quoting and escaping of compiler options Target-specific compiler options should be split into pre-processor defines, include directories, and additional options, then escaped/quoted and added to the appropriate portions of the project file. The "115 spaces backslash" test now checks that backslashes and spaces now work properly in all three places. --- mesonbuild/backend/vs2010backend.py | 49 +++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/mesonbuild/backend/vs2010backend.py b/mesonbuild/backend/vs2010backend.py index f7eb147ed..669bcf890 100644 --- a/mesonbuild/backend/vs2010backend.py +++ b/mesonbuild/backend/vs2010backend.py @@ -436,9 +436,30 @@ class Vs2010Backend(backends.Backend): # they are part of the CustomBuildStep Outputs. return - @classmethod - def quote_define_cmdline(cls, arg): - return re.sub(r'^([-/])D(.*?)="(.*)"$', r'\1D\2=\"\3\"', arg) + @staticmethod + def escape_preprocessor_define(define): + # See: https://msdn.microsoft.com/en-us/library/bb383819.aspx + table = str.maketrans({'%': '%25', '$': '%24', '@': '%40', + "'": '%27', ';': '%3B', '?': '%3F', '*': '%2A', + # We need to escape backslash because it'll be un-escaped by + # Windows during process creation when it parses the arguments + # Basically, this converts `\` to `\\`. + '\\': '\\\\'}) + return define.translate(table) + + @staticmethod + def escape_additional_option(option): + # See: https://msdn.microsoft.com/en-us/library/bb383819.aspx + table = str.maketrans({'%': '%25', '$': '%24', '@': '%40', + "'": '%27', ';': '%3B', '?': '%3F', '*': '%2A', ' ': '%20',}) + option = option.translate(table) + # Since we're surrounding the option with ", if it ends in \ that will + # escape the " when the process arguments are parsed and the starting + # " will not terminate. So we escape it if that's the case. I'm not + # kidding, this is how escaping works for process args on Windows. + if option.endswith('\\'): + option += '\\' + return '"{}"'.format(option) @staticmethod def split_link_args(args): @@ -633,7 +654,7 @@ class Vs2010Backend(backends.Backend): # so filter them out if needed d_compile_args = compiler.unix_compile_flags_to_native(d.get_compile_args()) for arg in d_compile_args: - if arg.startswith('-I'): + if arg.startswith('-I') or arg.startswith('/I'): inc_dir = arg[2:] # De-dup if inc_dir not in inc_dirs: @@ -641,8 +662,24 @@ class Vs2010Backend(backends.Backend): else: general_args.append(arg) + defines = [] + # Split preprocessor defines and include directories out of the list of + # all extra arguments. The rest go into %(AdditionalOptions). for l, args in extra_args.items(): - extra_args[l] = [Vs2010Backend.quote_define_cmdline(x) for x in args] + extra_args[l] = [] + for arg in args: + if arg.startswith('-D') or arg.startswith('/D'): + define = self.escape_preprocessor_define(arg[2:]) + # De-dup + if define not in defines: + defines.append(define) + elif arg.startswith('-I') or arg.startswith('/I'): + inc_dir = arg[2:] + # De-dup + if inc_dir not in inc_dirs: + inc_dirs.append(inc_dir) + else: + extra_args[l].append(self.escape_additional_option(arg)) languages += gen_langs has_language_specific_args = any(l != extra_args['c'] for l in extra_args.values()) @@ -669,7 +706,7 @@ class Vs2010Backend(backends.Backend): inc_dirs.append('%(AdditionalIncludeDirectories)') ET.SubElement(clconf, 'AdditionalIncludeDirectories').text = ';'.join(inc_dirs) - preproc = ET.SubElement(clconf, 'PreprocessorDefinitions') + ET.SubElement(clconf, 'PreprocessorDefinitions').text = ';'.join(defines) rebuild = ET.SubElement(clconf, 'MinimalRebuild') rebuild.text = 'true' funclink = ET.SubElement(clconf, 'FunctionLevelLinking') From bfa25fc1d344950751c8b0302ef289de9a23a749 Mon Sep 17 00:00:00 2001 From: Nirbheek Chauhan Date: Fri, 29 Jul 2016 19:50:36 +0530 Subject: [PATCH 3/3] ninja: Add escaping for backslash in -D arguments This is only needed for defines. Other arguments such as -I and /Fa that also take arguments with spaces and backslashes don't need it at all. --- mesonbuild/backend/backends.py | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/mesonbuild/backend/backends.py b/mesonbuild/backend/backends.py index d2693b252..1f1c3ca01 100644 --- a/mesonbuild/backend/backends.py +++ b/mesonbuild/backend/backends.py @@ -296,6 +296,33 @@ class Backend(): args = includeargs + args return args + @staticmethod + def escape_extra_args(compiler, args): + # No extra escaping/quoting needed when not running on Windows + if not mesonlib.is_windows(): + return args + extra_args = [] + # Compiler-specific escaping is needed for -D args but not for any others + if compiler.get_id() == 'msvc': + # MSVC needs escaping when a -D argument ends in \ or \" + for arg in args: + if arg.startswith('-D') or arg.startswith('/D'): + # Without extra escaping for these two, the next character + # gets eaten + if arg.endswith('\\'): + arg += '\\' + elif arg.endswith('\\"'): + arg = arg[:-2] + '\\\\"' + extra_args.append(arg) + else: + # MinGW GCC needs all backslashes in defines to be doubly-escaped + # FIXME: Not sure about Cygwin or Clang + for arg in args: + if arg.startswith('-D') or arg.startswith('/D'): + arg = arg.replace('\\', '\\\\') + extra_args.append(arg) + return extra_args + def generate_basic_compiler_args(self, target, compiler): commands = [] commands += self.get_cross_stdlib_args(target, compiler) @@ -304,7 +331,7 @@ class Backend(): commands += compiler.get_option_compile_args(self.environment.coredata.compiler_options) commands += self.build.get_global_args(compiler) commands += self.environment.coredata.external_args[compiler.get_language()] - commands += target.get_extra_args(compiler.get_language()) + commands += self.escape_extra_args(compiler, target.get_extra_args(compiler.get_language())) commands += compiler.get_buildtype_args(self.environment.coredata.get_builtin_option('buildtype')) if self.environment.coredata.get_builtin_option('werror'): commands += compiler.get_werror_args()