[clang][Headers] Do not define varargs macros for __need___va_list

[zatrazz]

Summary

The glibc uses the define to avoid namespace polution on headers
that requires variadic argument, where the inclusion of stdarg.h is
required to obtain the va_list definition.

For such cases only __gnuc_va_list is required.

[efriedma]

clang/lib/Headers/stdarg.h

19

Maybe the following is a little more readable?

#ifndef __STDARG_H

#ifndef __GNUC_VA_LIST
#define __GNUC_VA_LIST 1
typedef __builtin_va_list __gnuc_va_list;
#endif

#ifdef __need___va_list
#undef __need___va_list
#else
#define __STDARG_H
[...]

[MaskRay]

clang/lib/Headers/stdarg.h

15

To match gcc stdarg.h, #define __GNUC_VA_LIST

[MaskRay]

LGTM, but changes are needed.

[zatrazz]

clang/lib/Headers/stdarg.h

15

Ack.

19

Ack.

[zatrazz]

Update review based on reviewers comments.

[MaskRay]

LGTM.

[enh]

is there a corresponding glibc change so that va_list is exported for _POSIX_SOURCE cases? see https://android-review.git.corp.google.com/c/platform/bionic/+/2397313 where i'm having to disable some bionic testing against glibc because the glibc (2.17!) <wchar.h> now no longer exports va_list. i did look for a ToT glibc patch to backport (until we've _actually_ switched from glibc to musl for the host), but couldn't obviously find it?

https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/basedefs/wchar.h.html says:
"""
The <wchar.h> header shall define the following types:
...
va_list
[CX] As described in <stdarg.h>.
"""
which is why i think our "<wchar.h> exports va_list" test is correct. (Android doesn't support an "ISO only" mode --- you're effectively always in _POSIX_SOURCE mode, so we build the test against glibc with _POSIX_SOURCE defined.)

[zatrazz]

In D137268#4069779, @enh wrote:

is there a corresponding glibc change so that va_list is exported for _POSIX_SOURCE cases? see https://android-review.git.corp.google.com/c/platform/bionic/+/2397313 where i'm having to disable some bionic testing against glibc because the glibc (2.17!) <wchar.h> now no longer exports va_list. i did look for a ToT glibc patch to backport (until we've _actually_ switched from glibc to musl for the host), but couldn't obviously find it?

https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/basedefs/wchar.h.html says:
"""
The <wchar.h> header shall define the following types:
...
va_list
[CX] As described in <stdarg.h>.
"""
which is why i think our "<wchar.h> exports va_list" test is correct. (Android doesn't support an "ISO only" mode --- you're effectively always in _POSIX_SOURCE mode, so we build the test against glibc with _POSIX_SOURCE defined.)

I think I have caught this because your standard conformance tests checks for gnuc_va_list
on wchar.h, wich is always defined on on GCC (git log shows it was changed to fix XPG7
tests, but I am not sure exactly why the author has changed the va_list to gnuc_va_list).

And it seems that glibc seems broken also using GCC stdarg.h if I fix the test to check for
va_list instead. I will take a look at this.

[enh]

In D137268#4069856, @zatrazz wrote:

I think I have caught this because your standard conformance tests checks for gnuc_va_list
on wchar.h, wich is always defined on on GCC (git log shows it was changed to fix XPG7
tests, but I am not sure exactly why the author has changed the va_list to gnuc_va_list).

bionic's tests check for va_list, because that's what POSIX says will be visible. ISO C doesn't say that, so i think the _intention_ for glibc -- musl seems to do this correctly -- is to say "if we're only compiling C source, you don't get va_list, but if we're compiling POSIX source, you do get va_list". so i think this __gnuc_va_list thing is their workaround to still export the _functions_ without exporting the _type_ for ISO C?

here's the bionic *POSIX* test: https://cs.android.com/android/platform/superproject/+/master:bionic/tests/headers/posix/wchar_h.c;l=38 (but note that you'll have to look at the corresponding Android.bp file to see us define _POSIX_SOURCE.) note that our tests pass against _bionic_ (and, i think, musl). it's just old-glibc-with-new-llvm they fail against.

And it seems that glibc seems broken also using GCC stdarg.h if I fix the test to check for
va_list instead. I will take a look at this.

thanks!

[zatrazz]

In D137268#4069935, @enh wrote:

In D137268#4069856, @zatrazz wrote:

I think I have caught this because your standard conformance tests checks for gnuc_va_list
on wchar.h, wich is always defined on on GCC (git log shows it was changed to fix XPG7
tests, but I am not sure exactly why the author has changed the va_list to gnuc_va_list).

bionic's tests check for va_list, because that's what POSIX says will be visible. ISO C doesn't say that, so i think the _intention_ for glibc -- musl seems to do this correctly -- is to say "if we're only compiling C source, you don't get va_list, but if we're compiling POSIX source, you do get va_list". so i think this __gnuc_va_list thing is their workaround to still export the _functions_ without exporting the _type_ for ISO C?

Indeed the gnuc_va_list is used when building for c99/c11, since va_list should not be defined in such cases. It seems also that glibc stdio.h does handle it by defining gnuc_va_list to va_list if POSIX2001 or POSIX2008 is used (-D_XOPEN_SOURCE=600 or -D_XOPEN_SOURCE=700), so I think we should do the same for wchar.h

here's the bionic *POSIX* test: https://cs.android.com/android/platform/superproject/+/master:bionic/tests/headers/posix/wchar_h.c;l=38 (but note that you'll have to look at the corresponding Android.bp file to see us define _POSIX_SOURCE.) note that our tests pass against _bionic_ (and, i think, musl). it's just old-glibc-with-new-llvm they fail against.

And it seems that glibc seems broken also using GCC stdarg.h if I fix the test to check for
va_list instead. I will take a look at this.

thanks!

Could you check if this fixes your issue? I am not sure if if applies cleans to the ancient glibc 2.17 you are using:

diff --git a/conform/data/wchar.h-data b/conform/data/wchar.h-data
index e414651a33..582a99c00b 100644
--- a/conform/data/wchar.h-data
+++ b/conform/data/wchar.h-data
@@ -15,6 +15,9 @@ type size_t
 type locale_t
 # endif
 tag {struct tm}
+#if !defined ISO && !defined ISO99 && !defined ISO11 && !defined POSIX && !defined UNIX98
+type va_list
+#endif

 function wint_t btowc (int)
 function int fwprintf (FILE*, const wchar_t*, ...)
diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h
index 69e920b8c2..ca145bb8d2 100644
--- a/wcsmbs/wchar.h
+++ b/wcsmbs/wchar.h
@@ -37,6 +37,17 @@
 #define __need___va_list
 #include <stdarg.h>

+#if defined __USE_XOPEN2K || defined __USE_XOPEN2K8
+# ifdef __GNUC__
+#  ifndef _VA_LIST_DEFINED
+typedef __gnuc_va_list va_list;
+#   define _VA_LIST_DEFINED
+#  endif
+# else
+#  include <stdarg.h>
+# endif
+#endif
+
 #include <bits/wchar.h>
 #include <bits/types/wint_t.h>
 #include <bits/types/mbstate_t.h>

In any case, I will open a bug report on glibc fix on our side.

[enh]

In D137268#4069992, @zatrazz wrote:

Could you check if this fixes your issue?

yes, thanks... the person doing the llvm update tried it and reports that it works.

here's the patch against our old copy of glibc: https://android-review.googlesource.com/c/platform/prebuilts/gcc/linux-x86/host/x86_64-linux-glibc2.17-4.8/+/2403274

clang/lib/Headers/stdarg.h

 /*===---- stdarg.h - Variable argument handling ----------------------------===
  *
  * Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
  * See https://llvm.org/LICENSE.txt for license information.
  * SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
  *
  *===-----------------------------------------------------------------------===
  */
 
 #ifndef __STDARG_H
-#define __STDARG_H
 
+#ifndef __GNUC_VA_LIST
+#define __GNUC_VA_LIST
+typedef __builtin_va_list __gnuc_va_list;
+#endif
+
+#ifdef __need___va_list
+#undef __need___va_list
+#else
+#define __STDARG_H
 #ifndef _VA_LIST
 typedef __builtin_va_list va_list;
 #define _VA_LIST
 #endif
 #define va_start(ap, param) __builtin_va_start(ap, param)
 #define va_end(ap)          __builtin_va_end(ap)
 #define va_arg(ap, type)    __builtin_va_arg(ap, type)
 
 /* GCC always defines __va_copy, but does not define va_copy unless in c99 mode
  * or -ansi is not specified, since it was not part of C90.
  */
 #define __va_copy(d,s) __builtin_va_copy(d,s)
 
 #if (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) ||              \
     (defined(__cplusplus) && __cplusplus >= 201103L) ||                        \
     !defined(__STRICT_ANSI__)
 #define va_copy(dest, src)  __builtin_va_copy(dest, src)
 #endif
 
-#ifndef __GNUC_VA_LIST
-#define __GNUC_VA_LIST 1
-typedef __builtin_va_list __gnuc_va_list;
-#endif
-
 #endif /* __STDARG_H */
+
+#endif /* not __STDARG_H */

compiler-rt/test/sanitizer_common/TestCases/Linux/signal_send.cpp

 // RUN: %clangxx -std=c++11 -O0 -g %s -o %t && %run %t 2>&1 | FileCheck %s
 
 // sigandset is glibc specific.
 // UNSUPPORTED: android, freebsd, netbsd
 
 #include <assert.h>
 #include <signal.h>
+#include <stdarg.h>
 #include <stdio.h>
 #include <sys/time.h>
 #include <sys/wait.h>
 #include <unistd.h>
 
 sigset_t mkset(int n, ...) {
   sigset_t s;
   int res = 0;