Problems w/ ___errno on Solaris

[PHHargrove]

With clang-upc on Solaris/x86 and gcc or cc (Sun/Oracle compiler) as the back-end compiler, I see failures on upc_io_test.upc:

[benchmarks/upc_io_test] FAILED: UPC-To-C Translation or Link error     (NEW)
cd /shared/upcnightly-32/cupc2c/work/dbg/upc-tests/benchmarks
/export/home/phargrov/upcnightly-32/cupc2c/runtime/inst/bin/upcc -Ww,-Wno-duplicate-decl-specifier -Ww,-Werror=pointer-arith -g  -network=smp  -o upc_io_test upc_io_test.upc
upcc: error compiling translated C code:
upc_io_test.upc: In function 'doit':
upc_io_test.upc:200:13: warning: implicit declaration of function '___errno'
upc_io_test.upc:200:13: warning: nested extern declaration of '___errno' 
upc_io_test.upc:200:14: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:205:175: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:205:198: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:207:211: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:207:234: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:207:179: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:207:202: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:207:26: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:208:229: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:208:252: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:208:181: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:208:204: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:208:26: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:232:228: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:232:251: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:232:223: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:232:246: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:232:34: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:232:218: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:232:241: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:232:351: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:232:374: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:232:197: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:232:220: error: invalid type argument of unary '*' (have 'int')
upc_io_test.upc:232:26: error: invalid type argument of unary '*' (have 'int')
[... and LOTS more...]

The problem stems from the following in /usr/include/errno.h:

#if defined(_REENTRANT) || defined(_TS_ERRNO) || _POSIX_C_SOURCE - 0 >= 199506L
extern int *___errno();
#define errno (*(___errno()))
#else
extern int errno;
/* ANSI C++ requires that errno be a macro */
#if __cplusplus >= 199711L
#define errno errno
#endif
#endif  /* defined(_REENTRANT) || defined(_TS_ERRNO) */

It appears that clang (and thus clang-upc2c) defines _REENTRANT on Solaris, leading to incompatible differences between the first and second pre-process with respect tothe handling of errno.

This is odd because clang does not appear to unconditionally pre-define _REENTRANT on Linux, Darwin, FreeBSD, NetBSD or OpenBSD. Additionally, neither the vendor compiler nor gcc on Solaris are predefining it. So, this might simply by a side-effect of something that is already a "bug".

[PHHargrove]

This issue is (of course) not present with "-pthreads" since then _REENTRANT is defined consistently.

Looking deeper, the problem is only with "-network=smp" or "udp", but not "ibv". This is true only because ibv-conduit is adding -D_REENTRANT to the backend compiler flags. Similarly passing -Wc,-D_REENTRANT works for any network.

While sticking with -network=smp, compiling with -U_REENTRANT or -Wp,-U_REENTRANT do not work. Since passing -U_REENTRANT to the pre-process step (using -Wp,) did not work, I am uncertain as to how we might fix this issue properly. Clang-upc2c appears to be defining this despite a command-line argument to UNdefine it.

For the record, compiling the following confirms that it is only _REENTRANT that is leading to the errno definition:

#if defined(_TS_ERRNO) || _POSIX_C_SOURCE - 0 >= 199506L
  #error Not the fault of _REENTRANT
#endif

[swatanabe]

AMDG

On 03/02/2015 03:42 PM, Paul H. Hargrove wrote:

With clang-upc on Solaris/x86 and gcc or _clang_ as the back-end compiler,

It appears that **_clang**_ (and thus clang-upc2c) defines `_REENTRANT` on Solaris, leading to incompatible differences between the first and second pre-process with respect tothe handling of `errno`.

This really doesn't make sense to me. Does clang really

define _REENTRANT or is it only clang-upc2c? If clang

itself #defines _REENTRANT, then I would expect the test to pass with clang as the backend.

In Christ, Steven Watanabe

[PHHargrove]

Steven,

First, I need to make a correction: The test is OK with clang as the backend; I should have said that the failure was with gcc or _cc_ (Sun/Oracle compiler). The tests is fine with clang as the backend. Sorry for the confusion I caused.

Does clang really #define _REENTRANT or is it only clang-upc2c?

it is clang:

$ /shared/llvm-upc-32/bin/clang -dM -E -x c /dev/null | grep _REENTRANT
#define _REENTRANT 1

[nenadv]

Looking at the code, I see lots of:

    if (Opts.POSIXThreads)
      Builder.defineMacro("_REENTRANT");

All of them are like this, except for Solaris:

// Solaris target
template<typename Target>
class SolarisTargetInfo : public OSTargetInfo<Target> {
protected:
[...]
    Builder.defineMacro("__EXTENSIONS__");
    Builder.defineMacro("_REENTRANT");

[nenadv]

More on this here http://newsgroups.derkeiler.com/Archive/Comp/comp.programming.threads/2006-09/msg00018.html

And this post for gcc suggests that _REENTRANT is needed for Solaris thread? https://gcc.gnu.org/ml/gcc/2001-06/msg01589.html

Note that GCC has this for Solaris:

https://gcc.gnu.org/ml/gcc/2001-06/msg01589.html

[nenadv]

And GCC also condition _REENTRANT on -pthreads

#define CPP_SUBTARGET_SPEC "\
%{pthreads|pthread:-D_REENTRANT -D_PTHREADS}"

[swatanabe]

AMDG

On 03/03/2015 11:22 AM, Paul H. Hargrove wrote:

Steven,

First, I need to make a correction: The test is OK with clang as the backend; I should have said that the failure was with gcc or _cc_ (Sun/Oracle compiler). The tests is fine with clang as the backend. Sorry for the confusion I caused.

Thank you. That makes a lot more sense. It seems that Nenad has diagnosed the problem. I suppose we should take this to cfe-dev to find out if there's a good reason for them defining _REENTRANT unconditionally.

In Christ, Steven watanabe

[nenadv]

Paul, can you try the following patch on Solaris. This is the Clang repository.

diff --git a/lib/Basic/Targets.cpp b/lib/Basic/Targets.cpp
index 075f905..2571d4f 100644
--- a/lib/Basic/Targets.cpp
+++ b/lib/Basic/Targets.cpp
@@ -561,7 +561,8 @@ protected:
     Builder.defineMacro("_LARGEFILE_SOURCE");
     Builder.defineMacro("_LARGEFILE64_SOURCE");
     Builder.defineMacro("__EXTENSIONS__");
-    Builder.defineMacro("_REENTRANT");
+    if (Opts.POSIXThreads)
+      Builder.defineMacro("_REENTRANT");
   }
 public:
   SolarisTargetInfo(const llvm::Triple &Triple) : OSTargetInfo<Target>(Triple) {

[PHHargrove]

Paul, can you try the following patch on Solaris. This is the Clang repository.

Sure, it will take about 1.5hr to build. I will probably report after 2pm.

FYI: this is on pcp-j-19 and pcp-j-20 located "behind" n2001. You (Nenad) have accounts on both.

-Paul

[PHHargrove]

Nenad,

The patch did as it was intended, and now _REENTRANT is no longer defined unconditionally.

However, I find the original failure is still present. It appears that #include <upc_io.> results in defining _REENTRANT. I am investigating and assume this is the "fault" of UPCR.

-Paul

[PHHargrove]

OK, this is not as simple as expected. There is no longer a defn of _REENTRANT, and upc_io.h is only indirectly responsible. There is something in (or below) stdlib.h that is defining _POSIX_C_SOURCE to a value that leads to the thread-safe defn of errno and this is happening only on the first pre-process (w/ clang) but not on the second pre-process (with gcc or cc).

$ cat -n test.c
     1  #if defined _REENTRANT
     2    #warning _REENTRANT is defined before stdlib.h
     3  #endif
     4  #if defined _TS_ERRNO
     5    #warning _TS_ERRNOR is defined before stdlib.h
     6  #endif
     7  #if _POSIX_C_SOURCE - 0 >= 199506L
     8    #warning _POSIX_C_SOURCE >= 199506L before stdlib.h
     9  #endif
    10  #include <stdlib.h>
    11  #if defined _REENTRANT
    12    #warning _REENTRANT is defined after stdlib.h
    13  #endif
    14  #if defined _TS_ERRNO
    15    #warning _TS_ERRNOR is defined after stdlib.h
    16  #endif
    17  #if _POSIX_C_SOURCE - 0 >= 199506L
    18    #warning _POSIX_C_SOURCE >= 199506L after stdlib.h
    19  #endif
    20  
    21  #include <errno.h>
    22  #ifdef errno
    23  #warning Errno is now a macro
    24  #endif

$ clang -c test.c
test.c:18:4: warning: _POSIX_C_SOURCE >= 199506L after stdlib.h [-W#warnings]
  #warning _POSIX_C_SOURCE >= 199506L after stdlib.h
   ^
test.c:23:2: warning: Errno is now a macro [-W#warnings]
#warning Errno is now a macro
 ^
2 warnings generated.

$ clang -Wno-#warnings -dM -E test.c | grep _POSIX_C_SOURCE
#define _POSIX_C_SOURCE 200112L

The /usr/include/sys/feature_tests.h header appears to be the most likely spot for the definition - I am looking now.

[PHHargrove]

OK, the difference is _XOPEN_SOURCE defined by clang, but not by gcc or cc. The following comment in clang (a few lines before Nenad's patch for _REENTRANT) explains the motivation for the definition, though I am inclined to disagree with what is says since gcc -std=c99 and cc -xc99=all do NOT define _XOPEN_SOURCE at all (see test at end of this comment).

    // Solaris headers require _XOPEN_SOURCE to be set to 600 for C99 and
    // newer, but to 500 for everything else.  feature_test.h has a check to
    // ensure that you are not using C99 with an old version of X/Open or C89
    // with a new version.
    if (Opts.C99 || Opts.C11)
      Builder.defineMacro("_XOPEN_SOURCE", "600");
    else
      Builder.defineMacro("_XOPEN_SOURCE", "500");

The defn _XOPEN_SOURCE==600 leads to /usr/include/sys/feature_test.h defining _POSIX_C_SOURCE to 200112L, which in turn leads to the definition of the thread-safe errno. However, since gcc and cc do not define _XOPEN_SOURCE, the back-end pre-process does not follow the chain of definitions that leads to the thread-safe errno.

So, passing -U_XOPEN_SOURCE (to clang or clang-upc2c) is sufficient to work-around this problem. Of course so is -D_XOPEN_SOURCE=600 for the same reason -D_REENTRANT works - either one forces the backend compiler down the same path in errno.h as taken by the clang front end.

This interesting chain of defines also reveals why -U_REENTRANT was not sufficient in previous testing. One must remove both definitions for the first and second pre-process environments to be sufficiently similar.

Since we don't know if it is safe to modify SolarisTargetInfo::getOSDefines(), I don't see what is "the right path" at this point. If we learn that upstream will not change/remove these definitions then we are stuck with the possibility that we must add logic to undef _REENTRANT and _XOPEN_SOURCE. However, if we start down that path, then we end up with a subset UPCR's gcc_as_cc.pl script which finds all the -D, -U and -isystem flags needed to make gcc (or clang in this case) act exactly like the back-end preprocessor. However, that runs the risk of activating compiler-specific extensions (pragmas, non-Gnu asm, etc.) that would choke clang's parser.

Here is my test for definitions of _XOPEN_SOURCE:

$ cat -n test2.c
     1  #if _XOPEN_SOURCE - 600 == 0
     2    #warning _XOPEN_SOURCE 600
     3  #elif _XOPEN_SOURCE - 500 == 0
     4    #warning _XOPEN_SOURCE 500
     5  #elif defined _XOPEN_SOURCE
     6    #warning _XOPEN_SOURCE unknown
     7  #else
     8    #warning _XOPEN_SOURCE not defined
     9  #endif

$ clang -c test2.c 
test2.c:2:4: warning: _XOPEN_SOURCE 600 [-W#warnings]
  #warning _XOPEN_SOURCE 600
   ^
1 warning generated.

$ gcc -c -std=c99 test2.c
test2.c:8:4: warning: #warning _XOPEN_SOURCE not defined

$ cc -c -xc99=all test2.c
"test2.c", line 8: #warning: _XOPEN_SOURCE not defined
"test2.c", line 9: warning: empty translation unit

[nenadv]

I checked GCC and this seems to be defined only for C++

/* Names to predefine in the preprocessor for this target machine.  */
#define TARGET_SUB_OS_CPP_BUILTINS()
#define TARGET_OS_CPP_BUILTINS()                        \
    do {                                                \
        builtin_define_std ("unix");                    \
        builtin_define_std ("sun");                     \
        builtin_define ("__svr4__");                    \
        builtin_define ("__SVR4");                      \
        builtin_assert ("system=unix");                 \
        builtin_assert ("system=svr4");                 \
        /* For C++ we need to add some additional macro \
           definitions required by the C++ standard     \
           library.  */                                 \
        if (c_dialect_cxx ())                           \
          {                                             \
            builtin_define ("__STDC_VERSION__=199901L");\
            builtin_define ("_XOPEN_SOURCE=600");       \
            builtin_define ("_LARGEFILE_SOURCE=1");     \
            builtin_define ("_LARGEFILE64_SOURCE=1");   \
            builtin_define ("__EXTENSIONS__");          \
          }                                             \
        TARGET_SUB_OS_CPP_BUILTINS();                   \
    } while (0)

I think we should follow gcc in defining macros in Clang-UPC.

Paul, if you can do additional patch:

diff --git a/lib/Basic/Targets.cpp b/lib/Basic/Targets.cpp
index 075f905..3ff0833 100644
--- a/lib/Basic/Targets.cpp
+++ b/lib/Basic/Targets.cpp
@@ -548,20 +548,22 @@ protected:
     Builder.defineMacro("__ELF__");
     Builder.defineMacro("__svr4__");
     Builder.defineMacro("__SVR4");
-    // Solaris headers require _XOPEN_SOURCE to be set to 600 for C99 and
-    // newer, but to 500 for everything else.  feature_test.h has a check to
-    // ensure that you are not using C99 with an old version of X/Open or C89
-    // with a new version.  
-    if (Opts.C99 || Opts.C11)
-      Builder.defineMacro("_XOPEN_SOURCE", "600");
-    else
-      Builder.defineMacro("_XOPEN_SOURCE", "500");
-    if (Opts.CPlusPlus)
+    if (Opts.CPlusPlus) {
+      // Solaris headers require _XOPEN_SOURCE to be set to 600 for C99 and
+      // newer, but to 500 for everything else.  feature_test.h has a check to
+      // ensure that you are not using C99 with an old version of X/Open or C89
+      // with a new version.  
+      if (Opts.C99 || Opts.C11)
+        Builder.defineMacro("_XOPEN_SOURCE", "600");
+      else
+        Builder.defineMacro("_XOPEN_SOURCE", "500");
       Builder.defineMacro("__C99FEATURES__");
+    }
     Builder.defineMacro("_LARGEFILE_SOURCE");
     Builder.defineMacro("_LARGEFILE64_SOURCE");
     Builder.defineMacro("__EXTENSIONS__");
-    Builder.defineMacro("_REENTRANT");
+    if (Opts.POSIXThreads)
+      Builder.defineMacro("_REENTRANT");
   }

Paul, if you can please update the master, otherwise I'll do it, unless there are objections.

[PHHargrove]

Nenad,

Your patch is on my TODO list. However, our CVS server died on Sunday AM and resurecting it is a top priority at the moment,

-Paul

[PHHargrove]

Nenad,

For the record the gcc code you quoted also shows the following three as c++-only:

            builtin_define ("_LARGEFILE_SOURCE=1");     \
            builtin_define ("_LARGEFILE64_SOURCE=1");   \
            builtin_define ("__EXTENSIONS__");          \

[PHHargrove]

Nenad,

The patch to only define _XOPEN_SOURCE for C++ appears to have worked to resolve the problem. Since I am not working from a git checkout on the Solaris system, I've not committed to master.

I would suggest you also conditionalize the three defines I mentioned in the previous comment to make clang match gcc as closely as possible (even for tokens that haven't yet caused problems).

-Paul