Incorrect escaping in vendor.cc?

[daurnimator]

Trying to build on OSX, where CC=export MACOSX_DEPLOYMENT_TARGET=10.5; gcc results in the error:

mk/vendor.cc: line 7: export: `-E': not a valid identifier
mk/vendor.cc: line 7: export: `-': not a valid identifier

Running manually with bash -x:

$ env CC='export MACOSX_DEPLOYMENT_TARGET=10.5; gcc' bash -x mk/vendor.cc
+ set -e
+ : export 'MACOSX_DEPLOYMENT_TARGET=10.5;' gcc
+ export 'MACOSX_DEPLOYMENT_TARGET=10.5;' gcc -E -
+ awk '/sunpro/||/clang/||/gcc/||/other/{ print $1; exit; }'
+ MACOSX_DEPLOYMENT_TARGET='10.5;'
mk/vendor.cc: line 7: export: `-E': not a valid identifier
mk/vendor.cc: line 7: export: `-': not a valid identifier

[daurnimator]

So I tried working around this by using CC='env MACOSX_DEPLOYMENT_TARGET=10.5 gcc instead.

However that results in new errors in lua.path about the command 'MACOSX_DEPLOYMENT_TARGET=10.5' not being found. This seems to be an issue of incorrect quoting, which I fixed in ef4d38b66bf33fa3433155ace78da2da26914c50

[daurnimator]

This may all be mute, as gcc doesn't apparently support __thread on osx:

env MACOSX_DEPLOYMENT_TARGET=10.5 gcc -O2 -std=gnu99 -fPIC -g -Wall -Wextra -Wno-missing-field-initializers -Wno-initializer-overrides -Wno-unused -Wno-deprecated-declarations -O2 -fPIC -D_REENTRANT -D_THREAD_SAFE -D_GNU_SOURCE -DSOCKET_DEBUG -DDNS_RANDOM=arc4random  -c -o cqueues/src/lib/socket.o cqueues/src/lib/socket.c
cqueues/src/lib/socket.c:367:10: error: thread-local storage is unsupported for the current target
                static __thread char sslstr[256];

[daurnimator]

According to https://lists.apple.com/archives/Xcode-users/2006/Jun/msg00550.html you should use pthread_getspecific instead.

[wahern]

On Mon, Jan 12, 2015 at 12:51:18PM -0800, daurnimator wrote:

This may all be mute, as gcc doesn't apparently support __thread on osx:

env MACOSX_DEPLOYMENT_TARGET=10.5 gcc -O2 -std=gnu99 -fPIC -g -Wall -Wextra -Wno-missing-field-initializers -Wno-initializer-overrides -Wno-unused -Wno-deprecated-declarations -O2 -fPIC -D_REENTRANT -D_THREAD_SAFE -D_GNU_SOURCE -DSOCKET_DEBUG -DDNS_RANDOM=arc4random  -c -o cqueues/src/lib/socket.o cqueues/src/lib/socket.c
cqueues/src/lib/socket.c:367:10: error: thread-local storage is unsupported for the current target
                static __thread char sslstr[256];

Wow, that's a really old target. What version of OS X are you building on? OS X has had linker-supported TLS for quite a few releases and several years. GCC may have lagged a little bit in supporting it, but it's been so long I've forgotten the details. (Attached is a program verifying proper behavior, just 'cause you made me paranoid.)

The only system that still doesn't support __thread is OpenBSD. For OpenBSD I just disable thread support because threading is a mess there, anyhow.

Do you really need to target such an old release and including threading? pthread_getspecific is messy because you need to first initialize the key. To do that without requiring the application to explicitly initialize things, you need to call pthread_once prior to pthread_getspecific.

I guess supporting it isn't that big of deal because it's just in a [hopefully uncommon] error path. My only concern (other than the complexity) is that unlike with __thread, you need to always link in -lpthread. Which is a pain because then it can cause headaches in projects which don't care about threading. (I use socket.c elsewhere.) I'll probably have a preprocessor conditional which chooses one or the other method.

[daurnimator]

Wow, that's a really old target. What version of OS X are you building on?

OSX Yosemite. The target of 10.5 comes from luarocks: https://github.com/keplerproject/luarocks/blob/master/src/luarocks/cfg.lua#L451

OS X has had linker-supported TLS for quite a few releases and several years. GCC may have lagged a little bit in supporting it, but it's been so long I've forgotten the details.

$ gcc -v
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 6.0 (clang-600.0.56) (based on LLVM 3.5svn)
Target: x86_64-apple-darwin14.0.0
Thread model: posix

(Attached is a program verifying proper behavior, just 'cause you made me paranoid.)

Github strips attachments. Can you upload to a gist (or just email me privately?)

---

FWIW, clang compiles it fine.

So after some playing around, I discovered that it works fine with MACOSX_DEPLOYMENT_TARGET>=10.7. 10.5 and 10.6 do not like __thread.

[wahern]

#include <stdio.h>
#include <inttypes.h>
#include <string.h>
#include <pthread.h>

#define NTHREADS 8

static int file_scoped;

static void *f(void *_) {
    static volatile int running = 0;
    static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
    static pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
    static __thread int thread_scoped;
    static int func_scoped;
    int call_scoped;

    (void)_;

    pthread_mutex_lock(&mutex);
    if (++running == NTHREADS) {
        pthread_cond_broadcast(&cond);

        /* print header */
        printf("%-16s  %-16s  %-16s  %-16s\n",
            "file", "thread", "function", "call");
        for (int i = 0; i < 18 * 4; i++)
            putchar('-');
        putchar('\n');
    } else {
        pthread_cond_wait(&cond, &mutex);
    }
    pthread_mutex_unlock(&mutex);

    flockfile(stdout);
    printf("%.16"PRIxPTR"  %.16"PRIxPTR"  %.16"PRIxPTR"  %.16"PRIxPTR"\n",
        (uintptr_t)&file_scoped, (uintptr_t)&thread_scoped,
        (uintptr_t)&func_scoped, (uintptr_t)&call_scoped);
    funlockfile(stdout);

    pthread_mutex_lock(&mutex);
    if (--running == 0)
        pthread_cond_broadcast(&cond);
    else
        pthread_cond_wait(&cond, &mutex);
    pthread_mutex_unlock(&mutex);

    return 0;
}

int main(void) {
    for (int i = 0; i < NTHREADS - 1 /* exclude main thread */; i++)
        pthread_create(&(pthread_t){ 0 }, NULL, &f, 0);

    f(0);

    return 0;
}

[wahern]

Ouch. So it's LuaRocks using a compound command where a single command is expected. That's horrible. It's basically depending on buggy code--unintentional field expansion by the shell--for that hack to work.

[daurnimator]

I just posted a thread about the issue to the luarocks mailing list.

Do we have some minor things to fix with cqueue's makefile/build process?

[daurnimator]

I think this is fixed now.

[daurnimator]

Ouch. So it's LuaRocks using a compound command where a single command is expected. That's horrible. It's basically depending on buggy code--unintentional field expansion by the shell--for that hack to work.

luarocks has been fixed now (with 2.3.0 release); but cqueues has issues:

Luarocks runs: cd '/tmp/luarocks_cqueues-20150907.52-0-1999/cqueues-rel-20150907' && make -f GNUmakefile all5.2 'includedir=/usr/local/include' 'libdir=/usr/local/lib' 'CFLAGS=-O2 -fPIC' 'CC=env MACOSX_DEPLOYMENT_TARGET=10.5 gcc' 'bindir=/usr/local/bin'

Which fails with:

luapath: env MACOSX_DEPLOYMENT_TARGET=10.5 gcc: command not found
luapath: env MACOSX_DEPLOYMENT_TARGET=10.5 gcc: command not found
luapath: env MACOSX_DEPLOYMENT_TARGET=10.5 gcc: command not found
Unable to find Lua 5.2 API headers (CPPFLAGS=)

[daurnimator]

Possible fix:

diff --git a/mk/luapath b/mk/luapath
index 8df41c9..b8a3639 100755
--- a/mk/luapath
+++ b/mk/luapath
@@ -240,7 +240,7 @@ append() {
 #
 evalmacro() {
        printf "#include <$1>\n[===[$2]===]\n" \
-       | "${CC:-cc}" ${CPPFLAGS:-} -E - 2>>/dev/null \
+       | ${CC:-cc} ${CPPFLAGS:-} -E - 2>>/dev/null \
        | sed -ne "
                s/^.*\\[===\\[ *\\(${3:-.*}\\) *\\]===\\].*$/${4:-\\1}/
                t Found
@@ -1181,7 +1181,7 @@ shift $(($OPTIND - 1))

 for U in "${CC:-cc}" find grep od rm rmdir sed xargs; do
-       if ! command -v "${U}" >>/dev/null 2>&1; then
+       if ! command -v ${U} >>/dev/null 2>&1; then
                printf -- "${0##*/}: ${U}: command not found\n" >&2
        fi
 done

Once that's solved, on my Snow Leopard VM I run into the gcc error thread-local storage not supported for this target (doesn't happen with clang) when you use __thread (which brings us back to https://github.com/wahern/cqueues/issues/46#issuecomment-70146361). I don't have a newer OSX VM around, so will have to get someone else to test if this happens for newer OSX.

[daurnimator]

ping? (saw you commited some luapath fixes; but not this one)

[wahern]

I'm reticent to make that patch because I carefully wrote luapath to handle whitespace[1] within pathnames. For example, to be able to handle a gcc located at "/Applications/My Toolchain/bin/gcc". Of course, that example isn't practical because of the way most Makefiles are written. (And, admittedly, whitespace in autodetected include paths isn't escaped so it doesn't get split from -I.)

It just bugs me that luarocks requires hacks like that. The clean solution is to just invoke make with MACOSX_DEPLOYMENT_TARGET in the environment. Not only does it not make unwarranted assumptions about the structure of a variable string, it also ensures that every tool and utility has the same environment. For example, the linker[2] also needs to see MACOSX_DEPLOYMENT_TARGET, and conceivably an autoconf script might invoke the linker independently of the compiler.

Probably I'm just being unreasonable stubborn and should commit it. Though... all this complaining just gave me an idea: MACOSX_DEPLOYMENT_TARGET can be specified as a make variable and exported to subshell environments.

$ make MACOSX_DEPLOYMENT_TARGET=10.8

and from GNUmakefile

ifneq ($(origin MACOSX_DEPLOYMENT_TARGET), undefined)
export MACOSX_DEPLOYMENT_TARGET
endif

[1] As a general rule I try to write all my shell scripts to handle embedded whitespace in pathnames and similar values.

[2] Proof:

$ MACOSX_DEPLOYMENT_TARGET=10.8 gcc5 -c -o foo.o foo.c
$ ld -arch x86_64 -lSystem foo.o -o foo && ./foo
ld: warning: -macosx_version_min not specified, assuming 10.10

[daurnimator]

IMO the convention is that CC should be sent straight to the shell:

The built in make rules do this:

$ touch Makefile foo.c
$ make foo.o CC="echo hi"
echo hi    -c -o foo.o foo.c
hi -c -o foo.o foo.c

It's common pattern to use something like: make CC="gcc -march=myarch"

If you want to use a c compiler with a space in it, quote it as make CC='"/My Toolchain/gcc"'

[wahern]

On Tue, Mar 22, 2016 at 04:35:36PM -0700, daurnimator wrote:

IMO the convention is that CC should be sent straight to the shell:

In Makefiles, yes, if only because the habit is not to quote macros from Makefile commands. And arguably that's in part because different make implementations invoke the shell in different ways, so it was pointless to expect to be able to escape anything on behalf of the caller. That and because there aren't arrays, so you wanted CFLAGS to expand into multiple words.

My problem is allowing this poor form to carry over into shell scripts. As a general matter it's a security issue, especially when the argument is expanding into the command tobe invoked, as opposed to arguments.

I'll grant that in this case it's tolerable. It's just irksome because it's relying on messy string management that in almost every other context results in security nightmares.

autoconf, FWIW, generates conditional code to deal with a CC containing spaces or double quotes, though I couldn't quite make out all the logic.

The built in make rules do this:

$ touch Makefile foo.c
$ make foo.o CC="echo hi"
echo hi    -c -o foo.o foo.c
hi -c -o foo.o foo.c

It's common pattern to use something like: make CC="gcc -march=myarch"

That's what CFLAGS is for. And CPPFLAGS, for that matter, which pkg-config has eschewed. And in any event, for cross-compiling AFAIU it's more traditional to create a script to substitute for the CC command. The script ensures the environment is properly initialized, rather than trying to smuggle all of that stuff through make and relying on unwarranted assumptions about how CC is used beyond its invocation from the build rule.

Though it makes me wonder if I should also be passing ${CFLAGS} in mk/macros.ls and mk/luapath along with ${CPPFLAGS}. I wonder whether some valid flags (other than -c and -o) used for compilation might cause a pure preprocessor pass to fail. That's why I left them out.

If you want to use a c compiler with a space in it, quote it as make CC='"/My Toolchain/gcc"'

That solution works when invoking CC from GNU Make. But even if you pass CC directly to luapath, it won't work because the command isn't passing through system(3). Rather, when ${CC} is expanded, during the word splitting phase quotes are irrelevant. For example,

  #!/bin/sh
  count_args() {
    printf "\$# -> %d\n" "$#"
  }
  CC='"/My Toolchain/gcc"'
  count_args ${CC}

prints 2, not 1.

So supporting a path with whitespace and permitting multiple words to be passed via CC are mutually exclusive. I'll admit the latter is what people expect, though.