Re: [PATCH] winebuild: Use $CCAS to assemble if found

[Charles Davis]

Hi,

On Feb 3, 2013, at 7:33 PM, Alexandre Rostovtsev wrote:
> Commit c14bdaf1 made winebuild use Clang to assemble if found.
>
> However, just because a user has some version of Clang installed, it
> does not mean that she wants to use Clang to assemble Wine. For example,
> a user who has both Clang and GAS installed may want to use GAS to avoid
> textrels (see https://bugs.gentoo.org/show_bug.cgi?id=455308).

Long term, that should really get fixed in LLVM/Clang. I'll file a PR (if one wasn't already).
> This patch allows the user to override which assembler gets used by
> exporting CCAS at Wine configure time; the name CCAS was chosen for
> compatibility with automake's standard AM_PROG_AS macro.

I wrote the patch that prompted this one. And I'll freely admit, this is why I wanted to limit that patch to Mac OS--to minimize fallout on other platforms (like, obviously, Gentoo :).
> configure                   | 106 ++++++++++++++++++++++++++++++++++++++++++++
> configure.ac                |   4 ++
> tools/winebuild/Makefile.in |   5 ++-
> tools/winebuild/utils.c     |  12 +++++
> 4 files changed, 126 insertions(+), 1 deletion(-)
>
> diff --git a/configure b/configure
> index e3253ee..d0b7777 100755
> --- a/configure
> +++ b/configure
> @@ -732,6 +732,8 @@ FLEX
> TOOLSDIR
> WOW64_DISABLE
> TARGETFLAGS
> +ac_ct_CCAS
> +CCAS
> CPPBIN
> ac_ct_CXX
> CXXFLAGS
> @@ -861,6 +863,7 @@ CPPFLAGS
> CXX
> CXXFLAGS
> CCC
> +CCAS
> CPP
> XMKMF'
>
> @@ -1549,6 +1552,7 @@ Some influential environment variables:
>               you have headers in a nonstandard directory <include dir>
>   CXX         C++ compiler command
>   CXXFLAGS    C++ compiler flags
> - CCAS        Assembler command
> CPP         C preprocessor
> XMKMF       Path to xmkmf, Makefile generator for X Window System
>
> @@ -4075,6 +4079,108 @@ cat >>confdefs.h <<_ACEOF
> _ACEOF
> -
>
> +if test -n "$ac_tool_prefix"; then
> - for ac_prog in clang gas as
> - do
> - # Extract the first word of "$ac_tool_prefix$ac_prog", so it can be a program name with args.
>
> +set dummy $ac_tool_prefix$ac_prog; ac_word=$2
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5
> +$as_echo_n "checking for $ac_word... " >&6; }
> +if ${ac_cv_prog_CCAS+:} false; then :
> - $as_echo_n "(cached) " >&6
>
> +else
> - if test -n "$CCAS"; then
> - ac_cv_prog_CCAS="$CCAS" # Let the user override the test.
>
> +else
> +as_save_IFS=$IFS; IFS=$PATH_SEPARATOR
> +for as_dir in $PATH
> +do
> - IFS=$as_save_IFS
> - test -z "$as_dir" && as_dir=.
> - for ac_exec_ext in '' $ac_executable_extensions; do
> - if as_fn_executable_p "$as_dir/$ac_word$ac_exec_ext"; then
> - ac_cv_prog_CCAS="$ac_tool_prefix$ac_prog"
> - $as_echo "$as_me:${as_lineno-$LINENO}: found $as_dir/$ac_word$ac_exec_ext" >&5
> - break 2
> - fi
>
> +done
> - done
>
> +IFS=$as_save_IFS
> -
>
> +fi
> +fi
> +CCAS=$ac_cv_prog_CCAS
> +if test -n "$CCAS"; then
> - { $as_echo "$as_me:${as_lineno-$LINENO}: result: $CCAS" >&5
>
> +$as_echo "$CCAS" >&6; }
> +else
> - { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
>
> +$as_echo "no" >&6; }
> +fi
> -
> -
> - test -n "$CCAS" && break
> - done
>
> +fi
> +if test -z "$CCAS"; then
> - ac_ct_CCAS=$CCAS
> - for ac_prog in clang gas as
>
> +do
> - # Extract the first word of "$ac_prog", so it can be a program name with args.
>
> +set dummy $ac_prog; ac_word=$2
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5
> +$as_echo_n "checking for $ac_word... " >&6; }
> +if ${ac_cv_prog_ac_ct_CCAS+:} false; then :
> - $as_echo_n "(cached) " >&6
>
> +else
> - if test -n "$ac_ct_CCAS"; then
> - ac_cv_prog_ac_ct_CCAS="$ac_ct_CCAS" # Let the user override the test.
>
> +else
> +as_save_IFS=$IFS; IFS=$PATH_SEPARATOR
> +for as_dir in $PATH
> +do
> - IFS=$as_save_IFS
> - test -z "$as_dir" && as_dir=.
> - for ac_exec_ext in '' $ac_executable_extensions; do
> - if as_fn_executable_p "$as_dir/$ac_word$ac_exec_ext"; then
> - ac_cv_prog_ac_ct_CCAS="$ac_prog"
> - $as_echo "$as_me:${as_lineno-$LINENO}: found $as_dir/$ac_word$ac_exec_ext" >&5
> - break 2
> - fi
>
> +done
> - done
>
> +IFS=$as_save_IFS
> -
>
> +fi
> +fi
> +ac_ct_CCAS=$ac_cv_prog_ac_ct_CCAS
> +if test -n "$ac_ct_CCAS"; then
> - { $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_ct_CCAS" >&5
>
> +$as_echo "$ac_ct_CCAS" >&6; }
> +else
> - { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
>
> +$as_echo "no" >&6; }
> +fi
> -
> -
> - test -n "$ac_ct_CCAS" && break
>
> +done
> -
> - if test "x$ac_ct_CCAS" = x; then
> - CCAS=""$CC""
> - else
> - case $cross_compiling:$ac_tool_warned in
>
> +yes:)
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: using cross tools not prefixed with host triplet" >&5
> +$as_echo "$as_me: WARNING: using cross tools not prefixed with host triplet" >&2;}
> +ac_tool_warned=yes ;;
> +esac
> - CCAS=$ac_ct_CCAS
> - fi
>
> +fi
> -
> -
>
> case $host in
>   *-darwin*)
>     if test "x$enable_win64" = "xyes"
> diff --git a/configure.ac b/configure.ac
> index c93cd08..760f325 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -126,6 +126,10 @@ dnl We can't use AC_PROG_CPP for winegcc, it uses by default $(CC) -E
> AC_CHECK_TOOL(CPPBIN,cpp,cpp)
> AC_DEFINE_UNQUOTED(EXEEXT,["$ac_exeext"],[Define to the file extension for executables.])
>
> +dnl Analogous to AM_PROG_AS
> +AC_ARG_VAR([CCAS],[Assembler command])
> +AC_CHECK_TOOLS(CCAS,[clang gas as],["$CC"])

If the user doesn't set CCAS--which she doesn't the majority of the time--configure will pick up the first of {clang, gas, as} in the PATH. Are you sure that's what you want?

One of the earlier versions of the "Use Clang to assemble" patch attempted to detect Clang in the PATH, and use it if it was there. I was told that winebuild should detect the binary to use at runtime instead. Perhaps we should just not define CCAS if the user didn't specify it. Then below, you can wrap the block you added in an #ifdef.

AJ may have more to say about this.
> -
>
> case $host in
>   *-darwin*)
>     if test "x$enable_win64" = "xyes"
> diff --git a/tools/winebuild/Makefile.in b/tools/winebuild/Makefile.in
> index 2017129..3fe47a3 100644
> --- a/tools/winebuild/Makefile.in
> +++ b/tools/winebuild/Makefile.in
> @@ -1,4 +1,7 @@
> -DEFS      = -D__WINESRC__ $(EXTRADEFS)
> +DEFS      = \
> - -DCCAS=""@CCAS@"" \
> - -D__WINESRC__ \
> - $(EXTRADEFS)
>
> PROGRAMS = winebuild$(EXEEXT)
> MANPAGE  = winebuild.man
> diff --git a/tools/winebuild/utils.c b/tools/winebuild/utils.c
> index 262ff3a..1c5f918 100644
> --- a/tools/winebuild/utils.c
> +++ b/tools/winebuild/utils.c
> @@ -352,6 +352,18 @@ struct strarray *get_as_command(void)
>     static int as_is_clang = 0;
>     struct strarray *args = strarray_init();
> - if (!as_command && strlen( CCAS ))
> - {
> -
>    struct stat st;
> -
> -
>    if (!stat( CCAS, &st ))
> -
>        as_command = CCAS;
> -
>    else
> -
>        as_command = find_tool( CCAS, NULL );
> -
> -
>    if (as_command && strstr( as_command, "clang" )) as_is_clang = 1;

You may recall that one of the earlier versions (later than the one that detected Clang in configure) used strstr(3) to detect Clang; I was then told not to use it. I suspect this is because some systems' strstr(3) exhibits quadratic-time behavior. I don't know if you can avoid that in this case, though. Maybe you can just grab the basename and strip off the CTARGET prefix; then you should just be able to do a strcmp(3). Or am I missing something?

More generally, calling the variable "CCAS" implies that any old C compiler driver that knows how to assemble can be specified. If the user specifies a compiler that isn't Clang--GCC, for example--she'll get the wrong behavior. In particular, the 'as_is_clang' variable is used below to pass the right switches and options to Clang. Presumably we'd want to do that for the GCC case, too.
> - }
> - if (!as_command)
>   {
>       as_command = find_tool( "clang", NULL );
>
> -- 
> 1.8.1.2

I'm sorry for any inconvenience I might have caused, and I hope we can get this fixed soon.

Chip

[PATCH] winebuild: Use $CCAS to assemble if found

[Alexandre Rostovtsev]

On Sun, 2013-02-03 at 20:27 -0700, Charles Davis wrote:
> If the user doesn't set CCAS--which she doesn't the majority of the time--configure will pick up the first of {clang, gas, as} in the PATH. Are you sure that's what you want?

I had attempted to preserve the behavior that exists in wine-1.5.23 by
default because I assumed there was some good reason for it (even though
I could not see it). But if you agree that behavior was undesirable,
then of course it would be better to change the order depending on the
platform, and check for {clang, gas, as} on OSX, and {gas, as, clang} on
other platforms.
> Perhaps we should just not define CCAS if the user didn't specify it. Then below, you can wrap the block you added in an #ifdef.

My patch skips the CCAS block if strlen( CCAS ) == 0, which is basically
equivalent to what you propose.
> You may recall that one of the earlier versions (later than the one that detected Clang in configure) used strstr(3) to detect Clang; I was then told not to use it. I suspect this is because some systems' strstr(3) exhibits quadratic-time behavior. I don't know if you can avoid that in this case, though. Maybe you can just grab the basename and strip off the CTARGET prefix; then you should just be able to do a strcmp(3). Or am I missing something?

Even if strstr(x,y) is quadratic in strlen(x) and strlen(y), here it is
being called on two *constant* strings: CCAS and "clang". So the runtime
penalty is in fact constant :)

But it's probably better to avoid this penalty altogether. For example,
by checking in configure whether CCAS is Clang or GAS, and defining an
appropriate flag.

-Alexandre.

[PATCH] winebuild: Use $CCAS to assemble if found

[Charles Davis]

On Feb 3, 2013, at 9:36 PM, Alexandre Rostovtsev wrote:
> On Sun, 2013-02-03 at 20:27 -0700, Charles Davis wrote:
> > If the user doesn't set CCAS--which she doesn't the majority of the time--configure will pick up the first of {clang, gas, as} in the PATH. Are you sure that's what you want?
>
> I had attempted to preserve the behavior that exists in wine-1.5.23 by
> default because I assumed there was some good reason for it (even though
> I could not see it). But if you agree that behavior was undesirable,
> then of course it would be better to change the order depending on the
> platform, and check for {clang, gas, as} on OSX, and {gas, as, clang} on
> other platforms.

I considered doing that, but I figured it would make the code more complicated.
> > Perhaps we should just not define CCAS if the user didn't specify it. Then below, you can wrap the block you added in an #ifdef.
>
> My patch skips the CCAS block if strlen( CCAS ) == 0, which is basically
> equivalent to what you propose.

I don't think you understand exactly what AC_CHECK_TOOLS() does. If one of {clang, gas, as} exists in the path at *configure* time (prefixed with a CHOST triple or otherwise), strlen( CCAS ) *won't* be 0, because AC_CHECK_TOOLS() will pick one up. In fact, it will *never* be zero in this instance, because even if it doesn't find one of them, it'll just use the C compiler like you told it to.

And so I ask you again, are you sure you actually want to do this? Because I think you really don't. Otherwise, I wouldn't be asking :).

I'm sorry if I wasn't perfectly clear before.
> > You may recall that one of the earlier versions (later than the one that detected Clang in configure) used strstr(3) to detect Clang; I was then told not to use it. I suspect this is because some systems' strstr(3) exhibits quadratic-time behavior. I don't know if you can avoid that in this case, though. Maybe you can just grab the basename and strip off the CTARGET prefix; then you should just be able to do a strcmp(3). Or am I missing something?
>
> Even if strstr(x,y) is quadratic in strlen(x) and strlen(y), here it is
> being called on two *constant* strings: CCAS and "clang". So the runtime
> penalty is in fact constant :)

Only if the compiler knows how to optimize strstr(3) :).
> But it's probably better to avoid this penalty altogether. For example,
> by checking in configure whether CCAS is Clang or GAS, and defining an
> appropriate flag.

Go for it. Can't speak for AJ though; he has the final word on anything that goes in.

Chip
> -Alexandre.

[PATCH] winebuild: Use $CCAS to assemble if found

[Alexandre Julliard]

Charles Davis  writes:
> > But it's probably better to avoid this penalty altogether. For example,
> > by checking in configure whether CCAS is Clang or GAS, and defining an
> > appropriate flag.
>
> Go for it. Can't speak for AJ though; he has the final word on anything that goes in.

Like I already explained, this stuff needs to be done at run-time.
winebuild is a tool that can be used independently of Wine's configure.