| Message ID | 20231026234013.937210-1-thiago.bauermann@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | gdb/configure.ac: Fix auto-load options to work with Windows path separator | expand |
Hello, Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes: > Both --with-auto-load-dir and --with-auto-load-safe-path accept a list of > directories, which are separated by ':' on Unix and ';' on Windows. > However, as mentioned in PR 18898 this doesn't work on the latter OS. > > This is because the AC_DEFINE_DIR macro calls eval twice on the value > provided via these configure options, causing the ';' to be interpreted by > the shell. E.g., > > $ ~/src/binutils-gdb/configure \ > --disable-{binutils,ld,gold,gas,sim,gprof,gprofng} \ > --with-auto-load-dir='foo;bar' && make > ⋮ > checking for default auto-load directory... /home/bauermann/src/binutils-gdb/gdb/configure: line 18004: bar: command not found > foo;bar > checking for default auto-load safe-path... /home/bauermann/src/binutils-gdb/gdb/configure: line 18031: bar: command not found > foo;bar > > Line 18004 is: > > ac_define_dir=`eval echo $escape_dir` > > Line 18031 is identical. > > With some escaping, it's possible to avoid the problem: > > $ ~/src/binutils-gdb/configure \ > --disable-{binutils,ld,gold,gas,sim,gprof,gprofng} \ > --with-auto-load-dir='foo\\\;bar\\\;baz' && make > ⋮ > checking for default auto-load directory... foo\\\;bar\\\;baz > checking for default auto-load safe-path... foo\\\;bar\\\;baz > ⋮ > $ grep AUTO_LOAD gdb/config.h > #define AUTO_LOAD_DIR "foo;bar;baz" > #define AUTO_LOAD_SAFE_PATH "foo;bar;baz" > > We provide the definition of AC_DEFINE_DIR in gdb/acinclude.m4 so the > simplest approach to fix this problem would be to remove the evals from the > macro. > > I don't know why they are there however, so instead I decided to make > gdb/configure.ac automatically add those escaping characters to the values > of the options if the host OS is Windows. I don't know if Cygwin also > needs this change, so for now I'm only adding it for MinGW. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18898 > --- > > Hello, > > This addresses the problem that Eli has been having with these options > on MinGW. I don't have access to a Windows system, so I tested the patch > on Linux by changing the case to match "linux*" rather than "mingw*" and > using the build and grep commands mentioned in the commit message. > > It would be nice if someone could test on an actual Windows system, and > also let me know if other host_os strings should also be matched (e.g., > "cygwin*" or "windows*") > > gdb/configure | 20 ++++++++++++++++++-- > gdb/configure.ac | 20 ++++++++++++++++++-- > 2 files changed, 36 insertions(+), 4 deletions(-) Ping? As Eli mentioned on bug 18898, this patch is still needed. I confirmed that it still applies cleanly and works correctly.
Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes: > Both --with-auto-load-dir and --with-auto-load-safe-path accept a list of > directories, which are separated by ':' on Unix and ';' on Windows. > However, as mentioned in PR 18898 this doesn't work on the latter OS. > > This is because the AC_DEFINE_DIR macro calls eval twice on the value > provided via these configure options, causing the ';' to be interpreted by > the shell. E.g., > > $ ~/src/binutils-gdb/configure \ > --disable-{binutils,ld,gold,gas,sim,gprof,gprofng} \ > --with-auto-load-dir='foo;bar' && make > ⋮ > checking for default auto-load directory... /home/bauermann/src/binutils-gdb/gdb/configure: line 18004: bar: command not found > foo;bar > checking for default auto-load safe-path... /home/bauermann/src/binutils-gdb/gdb/configure: line 18031: bar: command not found > foo;bar > > Line 18004 is: > > ac_define_dir=`eval echo $escape_dir` > > Line 18031 is identical. > > With some escaping, it's possible to avoid the problem: > > $ ~/src/binutils-gdb/configure \ > --disable-{binutils,ld,gold,gas,sim,gprof,gprofng} \ > --with-auto-load-dir='foo\\\;bar\\\;baz' && make > ⋮ > checking for default auto-load directory... foo\\\;bar\\\;baz > checking for default auto-load safe-path... foo\\\;bar\\\;baz > ⋮ > $ grep AUTO_LOAD gdb/config.h > #define AUTO_LOAD_DIR "foo;bar;baz" > #define AUTO_LOAD_SAFE_PATH "foo;bar;baz" > > We provide the definition of AC_DEFINE_DIR in gdb/acinclude.m4 so the > simplest approach to fix this problem would be to remove the evals from the > macro. > > I don't know why they are there however, so instead I decided to make > gdb/configure.ac automatically add those escaping characters to the values > of the options if the host OS is Windows. I don't know if Cygwin also > needs this change, so for now I'm only adding it for MinGW. I tracked down a later version of AC_DEFINE_DIR (renamed to AC_DEFINE_DIR) in the autoconf-archive. The macro has actually been removed as it apparently violates some GNU guideline on how paths like this should be handled, but I didn't really understand... ...but the interesting bit is the definition of the macro just prior to its removal: AU_ALIAS([AC_DEFINE_DIR], [AX_DEFINE_DIR]) AC_DEFUN([AX_DEFINE_DIR], [ prefix_NONE= exec_prefix_NONE= test "x$prefix" = xNONE && prefix_NONE=yes && prefix=$ac_default_prefix test "x$exec_prefix" = xNONE && exec_prefix_NONE=yes && exec_prefix=$prefix dnl In Autoconf 2.60, ${datadir} refers to ${datarootdir}, which in turn dnl refers to ${prefix}. Thus we have to use `eval' twice. eval ax_define_dir="\"[$]$2\"" eval ax_define_dir="\"$ax_define_dir\"" AC_SUBST($1, "$ax_define_dir") AC_DEFINE_UNQUOTED($1, "$ax_define_dir", [$3]) test "$prefix_NONE" && prefix=NONE test "$exec_prefix_NONE" && exec_prefix=NONE ]) Not only does this offer a little insight into why the double eval exists, but I notice the use of eval has changed slightly, and there are now quotes being used, e.g. "\"[$]$2\"" I wonder if using some of the updates would offer an alternative solution? Thanks, Andrew > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18898 > --- > > Hello, > > This addresses the problem that Eli has been having with these options > on MinGW. I don't have access to a Windows system, so I tested the patch > on Linux by changing the case to match "linux*" rather than "mingw*" and > using the build and grep commands mentioned in the commit message. > > It would be nice if someone could test on an actual Windows system, and > also let me know if other host_os strings should also be matched (e.g., > "cygwin*" or "windows*") > > gdb/configure | 20 ++++++++++++++++++-- > gdb/configure.ac | 20 ++++++++++++++++++-- > 2 files changed, 36 insertions(+), 4 deletions(-) > > diff --git a/gdb/configure b/gdb/configure > index 5361bf42952a..96f0e3da9637 100755 > --- a/gdb/configure > +++ b/gdb/configure > @@ -17997,7 +17997,15 @@ else > with_auto_load_dir='$debugdir:$datadir/auto-load' > fi > > -escape_dir=`echo $with_auto_load_dir | sed -e 's/[$]datadir\>/\\\\\\\\\\\\&/g' -e 's/[$]debugdir\>/\\\\\\\\\\\\&/g'` > +# If we're on Windows, we need to protect the PATH separator ';' from the couple > +# of eval calls done by AC_DEFINE_DIR. > +extra_sed_args=() > +case "$host_os" in > + mingw*) > + extra_sed_args=(-e 's/;/\\\\\\;/g') > + ;; > +esac > +escape_dir=`echo $with_auto_load_dir | sed -e 's/[$]datadir\>/\\\\\\\\\\\\&/g' -e 's/[$]debugdir\>/\\\\\\\\\\\\&/g' "${extra_sed_args[@]}"` > > test "x$prefix" = xNONE && prefix="$ac_default_prefix" > test "x$exec_prefix" = xNONE && exec_prefix='${prefix}' > @@ -18024,7 +18032,15 @@ else > with_auto_load_safe_path="$with_auto_load_dir" > fi > > -escape_dir=`echo $with_auto_load_safe_path | sed -e 's/[$]datadir\>/\\\\\\\\\\\\&/g' -e 's/[$]debugdir\>/\\\\\\\\\\\\&/g'` > +# If we're on Windows, we need to protect the PATH separator ';' from the couple > +# of eval calls done by AC_DEFINE_DIR. > +extra_sed_args=() > +case "$host_os" in > + mingw*) > + extra_sed_args=(-e 's/;/\\\\\\;/g') > + ;; > +esac > +escape_dir=`echo $with_auto_load_safe_path | sed -e 's/[$]datadir\>/\\\\\\\\\\\\&/g' -e 's/[$]debugdir\>/\\\\\\\\\\\\&/g' "${extra_sed_args[@]}"` > > test "x$prefix" = xNONE && prefix="$ac_default_prefix" > test "x$exec_prefix" = xNONE && exec_prefix='${prefix}' > diff --git a/gdb/configure.ac b/gdb/configure.ac > index 3912b77b27f2..cc8d2bfc5e8b 100644 > --- a/gdb/configure.ac > +++ b/gdb/configure.ac > @@ -153,7 +153,15 @@ AC_ARG_WITH(auto-load-dir, > AS_HELP_STRING([--with-auto-load-dir=PATH], > [directories from which to load auto-loaded scripts @<:@$debugdir:$datadir/auto-load@:>@]),, > [with_auto_load_dir='$debugdir:$datadir/auto-load']) > -escape_dir=`echo $with_auto_load_dir | sed -e 's/[[$]]datadir\>/\\\\\\\\\\\\&/g' -e 's/[[$]]debugdir\>/\\\\\\\\\\\\&/g'` > +# If we're on Windows, we need to protect the PATH separator ';' from the couple > +# of eval calls done by AC_DEFINE_DIR. > +extra_sed_args=() > +case "$host_os" in > + mingw*) > + extra_sed_args=(-e 's/;/\\\\\\;/g') > + ;; > +esac > +escape_dir=`echo $with_auto_load_dir | sed -e 's/[[$]]datadir\>/\\\\\\\\\\\\&/g' -e 's/[[$]]debugdir\>/\\\\\\\\\\\\&/g' "${extra_sed_args[[@]]}"` > AC_DEFINE_DIR(AUTO_LOAD_DIR, escape_dir, > [Directories from which to load auto-loaded scripts.]) > AC_MSG_RESULT([$with_auto_load_dir]) > @@ -168,7 +176,15 @@ AS_HELP_STRING([--without-auto-load-safe-path], > with_auto_load_safe_path="/" > fi], > [with_auto_load_safe_path="$with_auto_load_dir"]) > -escape_dir=`echo $with_auto_load_safe_path | sed -e 's/[[$]]datadir\>/\\\\\\\\\\\\&/g' -e 's/[[$]]debugdir\>/\\\\\\\\\\\\&/g'` > +# If we're on Windows, we need to protect the PATH separator ';' from the couple > +# of eval calls done by AC_DEFINE_DIR. > +extra_sed_args=() > +case "$host_os" in > + mingw*) > + extra_sed_args=(-e 's/;/\\\\\\;/g') > + ;; > +esac > +escape_dir=`echo $with_auto_load_safe_path | sed -e 's/[[$]]datadir\>/\\\\\\\\\\\\&/g' -e 's/[[$]]debugdir\>/\\\\\\\\\\\\&/g' "${extra_sed_args[[@]]}"` > AC_DEFINE_DIR(AUTO_LOAD_SAFE_PATH, escape_dir, > [Directories safe to hold auto-loaded files.]) > AC_MSG_RESULT([$with_auto_load_safe_path])
Hello Andrew, Andrew Burgess <aburgess@redhat.com> writes: > Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes: > >> Both --with-auto-load-dir and --with-auto-load-safe-path accept a list of >> directories, which are separated by ':' on Unix and ';' on Windows. >> However, as mentioned in PR 18898 this doesn't work on the latter OS. >> >> This is because the AC_DEFINE_DIR macro calls eval twice on the value >> provided via these configure options, causing the ';' to be interpreted by >> the shell. E.g., >> >> $ ~/src/binutils-gdb/configure \ >> --disable-{binutils,ld,gold,gas,sim,gprof,gprofng} \ >> --with-auto-load-dir='foo;bar' && make >> ⋮ >> checking for default auto-load directory... /home/bauermann/src/binutils-gdb/gdb/configure: line 18004: bar: command not found >> foo;bar >> checking for default auto-load safe-path... /home/bauermann/src/binutils-gdb/gdb/configure: line 18031: bar: command not found >> foo;bar >> >> Line 18004 is: >> >> ac_define_dir=`eval echo $escape_dir` >> >> Line 18031 is identical. >> >> With some escaping, it's possible to avoid the problem: >> >> $ ~/src/binutils-gdb/configure \ >> --disable-{binutils,ld,gold,gas,sim,gprof,gprofng} \ >> --with-auto-load-dir='foo\\\;bar\\\;baz' && make >> ⋮ >> checking for default auto-load directory... foo\\\;bar\\\;baz >> checking for default auto-load safe-path... foo\\\;bar\\\;baz >> ⋮ >> $ grep AUTO_LOAD gdb/config.h >> #define AUTO_LOAD_DIR "foo;bar;baz" >> #define AUTO_LOAD_SAFE_PATH "foo;bar;baz" >> >> We provide the definition of AC_DEFINE_DIR in gdb/acinclude.m4 so the >> simplest approach to fix this problem would be to remove the evals from the >> macro. >> >> I don't know why they are there however, so instead I decided to make >> gdb/configure.ac automatically add those escaping characters to the values >> of the options if the host OS is Windows. I don't know if Cygwin also >> needs this change, so for now I'm only adding it for MinGW. > > I tracked down a later version of AC_DEFINE_DIR (renamed to > AC_DEFINE_DIR) in the autoconf-archive. The macro has actually been > removed as it apparently violates some GNU guideline on how paths like > this should be handled, but I didn't really understand... > > ...but the interesting bit is the definition of the macro just prior to > its removal: > > AU_ALIAS([AC_DEFINE_DIR], [AX_DEFINE_DIR]) > AC_DEFUN([AX_DEFINE_DIR], [ > prefix_NONE= > exec_prefix_NONE= > test "x$prefix" = xNONE && prefix_NONE=yes && prefix=$ac_default_prefix > test "x$exec_prefix" = xNONE && exec_prefix_NONE=yes && exec_prefix=$prefix > dnl In Autoconf 2.60, ${datadir} refers to ${datarootdir}, which in turn > dnl refers to ${prefix}. Thus we have to use `eval' twice. > eval ax_define_dir="\"[$]$2\"" > eval ax_define_dir="\"$ax_define_dir\"" > AC_SUBST($1, "$ax_define_dir") > AC_DEFINE_UNQUOTED($1, "$ax_define_dir", [$3]) > test "$prefix_NONE" && prefix=NONE > test "$exec_prefix_NONE" && exec_prefix=NONE > ]) Ah! I looked for it in autoconf-archive but didn't think of looking at the repo history, so I gave up. Thanks! > Not only does this offer a little insight into why the double eval > exists, but I notice the use of eval has changed slightly, and there are > now quotes being used, e.g. "\"[$]$2\"" > > I wonder if using some of the updates would offer an alternative > solution? Indeed! Using the definition above fixed the problem. I'll send an updated patch.
diff --git a/gdb/configure b/gdb/configure index 5361bf42952a..96f0e3da9637 100755 --- a/gdb/configure +++ b/gdb/configure @@ -17997,7 +17997,15 @@ else with_auto_load_dir='$debugdir:$datadir/auto-load' fi -escape_dir=`echo $with_auto_load_dir | sed -e 's/[$]datadir\>/\\\\\\\\\\\\&/g' -e 's/[$]debugdir\>/\\\\\\\\\\\\&/g'` +# If we're on Windows, we need to protect the PATH separator ';' from the couple +# of eval calls done by AC_DEFINE_DIR. +extra_sed_args=() +case "$host_os" in + mingw*) + extra_sed_args=(-e 's/;/\\\\\\;/g') + ;; +esac +escape_dir=`echo $with_auto_load_dir | sed -e 's/[$]datadir\>/\\\\\\\\\\\\&/g' -e 's/[$]debugdir\>/\\\\\\\\\\\\&/g' "${extra_sed_args[@]}"` test "x$prefix" = xNONE && prefix="$ac_default_prefix" test "x$exec_prefix" = xNONE && exec_prefix='${prefix}' @@ -18024,7 +18032,15 @@ else with_auto_load_safe_path="$with_auto_load_dir" fi -escape_dir=`echo $with_auto_load_safe_path | sed -e 's/[$]datadir\>/\\\\\\\\\\\\&/g' -e 's/[$]debugdir\>/\\\\\\\\\\\\&/g'` +# If we're on Windows, we need to protect the PATH separator ';' from the couple +# of eval calls done by AC_DEFINE_DIR. +extra_sed_args=() +case "$host_os" in + mingw*) + extra_sed_args=(-e 's/;/\\\\\\;/g') + ;; +esac +escape_dir=`echo $with_auto_load_safe_path | sed -e 's/[$]datadir\>/\\\\\\\\\\\\&/g' -e 's/[$]debugdir\>/\\\\\\\\\\\\&/g' "${extra_sed_args[@]}"` test "x$prefix" = xNONE && prefix="$ac_default_prefix" test "x$exec_prefix" = xNONE && exec_prefix='${prefix}' diff --git a/gdb/configure.ac b/gdb/configure.ac index 3912b77b27f2..cc8d2bfc5e8b 100644 --- a/gdb/configure.ac +++ b/gdb/configure.ac @@ -153,7 +153,15 @@ AC_ARG_WITH(auto-load-dir, AS_HELP_STRING([--with-auto-load-dir=PATH], [directories from which to load auto-loaded scripts @<:@$debugdir:$datadir/auto-load@:>@]),, [with_auto_load_dir='$debugdir:$datadir/auto-load']) -escape_dir=`echo $with_auto_load_dir | sed -e 's/[[$]]datadir\>/\\\\\\\\\\\\&/g' -e 's/[[$]]debugdir\>/\\\\\\\\\\\\&/g'` +# If we're on Windows, we need to protect the PATH separator ';' from the couple +# of eval calls done by AC_DEFINE_DIR. +extra_sed_args=() +case "$host_os" in + mingw*) + extra_sed_args=(-e 's/;/\\\\\\;/g') + ;; +esac +escape_dir=`echo $with_auto_load_dir | sed -e 's/[[$]]datadir\>/\\\\\\\\\\\\&/g' -e 's/[[$]]debugdir\>/\\\\\\\\\\\\&/g' "${extra_sed_args[[@]]}"` AC_DEFINE_DIR(AUTO_LOAD_DIR, escape_dir, [Directories from which to load auto-loaded scripts.]) AC_MSG_RESULT([$with_auto_load_dir]) @@ -168,7 +176,15 @@ AS_HELP_STRING([--without-auto-load-safe-path], with_auto_load_safe_path="/" fi], [with_auto_load_safe_path="$with_auto_load_dir"]) -escape_dir=`echo $with_auto_load_safe_path | sed -e 's/[[$]]datadir\>/\\\\\\\\\\\\&/g' -e 's/[[$]]debugdir\>/\\\\\\\\\\\\&/g'` +# If we're on Windows, we need to protect the PATH separator ';' from the couple +# of eval calls done by AC_DEFINE_DIR. +extra_sed_args=() +case "$host_os" in + mingw*) + extra_sed_args=(-e 's/;/\\\\\\;/g') + ;; +esac +escape_dir=`echo $with_auto_load_safe_path | sed -e 's/[[$]]datadir\>/\\\\\\\\\\\\&/g' -e 's/[[$]]debugdir\>/\\\\\\\\\\\\&/g' "${extra_sed_args[[@]]}"` AC_DEFINE_DIR(AUTO_LOAD_SAFE_PATH, escape_dir, [Directories safe to hold auto-loaded files.]) AC_MSG_RESULT([$with_auto_load_safe_path])
Both --with-auto-load-dir and --with-auto-load-safe-path accept a list of directories, which are separated by ':' on Unix and ';' on Windows. However, as mentioned in PR 18898 this doesn't work on the latter OS. This is because the AC_DEFINE_DIR macro calls eval twice on the value provided via these configure options, causing the ';' to be interpreted by the shell. E.g., $ ~/src/binutils-gdb/configure \ --disable-{binutils,ld,gold,gas,sim,gprof,gprofng} \ --with-auto-load-dir='foo;bar' && make ⋮ checking for default auto-load directory... /home/bauermann/src/binutils-gdb/gdb/configure: line 18004: bar: command not found foo;bar checking for default auto-load safe-path... /home/bauermann/src/binutils-gdb/gdb/configure: line 18031: bar: command not found foo;bar Line 18004 is: ac_define_dir=`eval echo $escape_dir` Line 18031 is identical. With some escaping, it's possible to avoid the problem: $ ~/src/binutils-gdb/configure \ --disable-{binutils,ld,gold,gas,sim,gprof,gprofng} \ --with-auto-load-dir='foo\\\;bar\\\;baz' && make ⋮ checking for default auto-load directory... foo\\\;bar\\\;baz checking for default auto-load safe-path... foo\\\;bar\\\;baz ⋮ $ grep AUTO_LOAD gdb/config.h #define AUTO_LOAD_DIR "foo;bar;baz" #define AUTO_LOAD_SAFE_PATH "foo;bar;baz" We provide the definition of AC_DEFINE_DIR in gdb/acinclude.m4 so the simplest approach to fix this problem would be to remove the evals from the macro. I don't know why they are there however, so instead I decided to make gdb/configure.ac automatically add those escaping characters to the values of the options if the host OS is Windows. I don't know if Cygwin also needs this change, so for now I'm only adding it for MinGW. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18898 --- Hello, This addresses the problem that Eli has been having with these options on MinGW. I don't have access to a Windows system, so I tested the patch on Linux by changing the case to match "linux*" rather than "mingw*" and using the build and grep commands mentioned in the commit message. It would be nice if someone could test on an actual Windows system, and also let me know if other host_os strings should also be matched (e.g., "cygwin*" or "windows*") gdb/configure | 20 ++++++++++++++++++-- gdb/configure.ac | 20 ++++++++++++++++++-- 2 files changed, 36 insertions(+), 4 deletions(-)