problem (and fix) with -fms-extensions

I've tried to build something that wanted ms-extensions on OpenBSD.
Long story short, didn't work so well, because all system includes
lead to

<machine/_types.h>
#ifndef __cplusplus
typedef int __wchar_t;
#endif

and since ms-extensions includes __char_t as a built-in, this did fail
abysmally.

It would be simple to fix in OpenBSD, assuming clang did tell us it
was using ms-extensions.

Would something like this be appropriate ? macro name subject to approval
of course.

Index: lib/Frontend/InitPreprocessor.cpp

ppl (721 Bytes)

I've tried to build something that wanted ms-extensions on OpenBSD.
Long story short, didn't work so well, because all system includes
lead to

<machine/_types.h>
#ifndef __cplusplus
typedef int __wchar_t;
#endif

and since ms-extensions includes __char_t as a built-in, this did fail
abysmally.

Back in 2014 when we encountered this in FreeBSD, we just renamed our
internal type to ___wchar_t instead:

http://svnweb.freebsd.org/changeset/base/263998

It would be simple to fix in OpenBSD, assuming clang did tell us it
was using ms-extensions.

Would something like this be appropriate ? macro name subject to approval
of course.

Index: lib/Frontend/InitPreprocessor.cpp

RCS file: /build/data/openbsd/cvs/src/gnu/llvm/tools/clang/lib/Frontend/InitPreprocessor.cpp,v
retrieving revision 1.1.1.4
diff -u -p -r1.1.1.4 InitPreprocessor.cpp
--- lib/Frontend/InitPreprocessor.cpp 14 Mar 2017 08:07:56 -0000 1.1.1.4
+++ lib/Frontend/InitPreprocessor.cpp 11 May 2017 17:54:41 -0000
@@ -366,6 +366,8 @@ static void InitializeStandardPredefined
  else
    Builder.defineMacro("__STDC_HOSTED__");

+ if (LangOpts.MicrosoftMode)
+ Builder.defineMacro("__ms_extensions__", 1);
  if (!LangOpts.CPlusPlus) {
    if (LangOpts.C11)
      Builder.defineMacro("__STDC_VERSION__", "201112L");

Looks fine to me, though you would also have to convince the gcc authors
to do the same. (That is, if you still want to use recent gcc's... :slight_smile:

-Dimitry

Well, the licence means we care a little less about gcc for obvious reasons.
Working with the llvm/clang community makes more sense for us.

...

+ if (LangOpts.MicrosoftMode)
+ Builder.defineMacro("__ms_extensions__", 1);
if (!LangOpts.CPlusPlus) {
   if (LangOpts.C11)
     Builder.defineMacro("__STDC_VERSION__", "201112L");

Looks fine to me, though you would also have to convince the gcc authors
to do the same. (That is, if you still want to use recent gcc's... :slight_smile:

Well, the licence means we care a little less about gcc for obvious reasons.
Working with the llvm/clang community makes more sense for us.

Another option, which works right away, without having to modify any
compiler, is to just define it on the command line, e.g. use:
-fms-extensions -D__ms_extensions__

-Dimitry

Let's make it clear, I'm not looking for a work-around, I'm looking for a way
to fix things out-of-the-box. Having a "by default" define means we can
fix the includes once and for all, and have clang -fms-extensions work
out-of-the-box without any configure magic.

People who deal with portable code know the pain of having some code that
doesn't work on YOUR system because it requires one extra step that the
people upstream don't know about, so you have to make a patch, pass it
upstream... and often have things fail again down the line because upstream
dropped your change because no-one there knew what it was for.

Or try to figure out where you can inject the patch in some crazy build
system you have never worked with, and that actively fights you all the
way...

Just to make it clear where I'm coming from, over the past few weeks,
I've fixed about 200 stupid configury bugs for clang, between the
stuff that put int main(int argc, char *argv[]) in configure.ac,
the stuff that asserts that $(CC) -Wwhatever works since the compiler
didn't error out, the crazy mingw build shell script that won't
insert -fgnu89-inline just for the gcc3.4 bootstrap stage, or
python2.7 that by default disables multibyte functions required for
__bsd_locale_fallbacks.h ...

Moving to cfe-dev to discuss clang things.

I think we already have _WCHAR_T_DEFINED for this:

if (LangOpts.MicrosoftExt) {
if (LangOpts.WChar) {
// wchar_t supported as a keyword.
Builder.defineMacro("_WCHAR_T_DEFINED");
Builder.defineMacro("_NATIVE_WCHAR_T_DEFINED");
}
}

Nope. I'm talking about the __wchar_t type.
In my case, even a C language "hello world" won't compile:

nausicaa$ clang -fms-extensions a.c
In file included from a.c:1:
In file included from /usr/include/stdio.h:43:
In file included from /usr/include/sys/_types.h:37:
/usr/include/machine/_types.h:132:15: error: cannot combine with previous 'int'
      declaration specifier
typedef int __wchar_t;
                                ^
1 error generated.

but actually based on the existing code, I would suggest following
the pattern.

my main concern would be: are names with ___ readable, or is there
a suggestion for better names ?

Index: InitPreprocessor.cpp

I’d rather not add new pre-defined macros if we can avoid it. There are already too many. You can probably detect this situation with:
#if defined(_MSC_EXTENSIONS) && !defined(_WCHAR_T_DEFINED)

Was this added recently ?

There is no _MSC_EXTENSIONS in the clang I'm using...

Looks like we only define it for Windows targets.

How about __is_identifier?

$ cat t.cpp
#if !__is_identifier(__wchar_t)
#error "have __wchar_t"
#else
#error "no __wchar_t"
#endif

$ clang --target=x86_64-linux -c t.cpp
t.cpp:4:2: error: "no __wchar_t"
#error "no __wchar_t"
^
1 error generated.

$ clang --target=x86_64-linux -c t.cpp -fms-extensions
t.cpp:2:2: error: "have __wchar_t"
#error "have __wchar_t"
^
1 error generated.

Nope, that will trigger errors on non clang compilers.

Why do you fight so hard against a simple solution ?

Or, what's the problem with moving _MSC_EXTENSIONS to be for all targets.

Look, this stuff is highly specific, I doubt many people are going to
use -fms-extensions who don't know what they're doing.
E.g., the exposure to _MSC_EXTENSIONS will be minimal.

Having a single define will mean replacing (for us)
#ifndef __cplusplus
typedef int __wchar_t;
#endif

with
#if !defined(__cplusplus) && !defined(_MSC_EXTENSIONS)
typedef int __wchar_t;
#endif

which is fairly acceptable.

Playing with __is_identifier is rather more awful, because there is no
simple construct, we can't simply write:
#if !defined(__cplusplus) || __is_identifier(__wchar_t)
because this *also* has to work with other compilers, some of which
do not understand __is_identifier at all, so it can't be a single test
then.

It would become some atrocity like

#if !defined(__cplusplus)
# if defined(__is_identifier)
# if __is_identifier(__wchar_t)
# else
typedef int __wchar_t;
# endif
# else
typedef int __wchar_t;
# endif
#endif

And that has about zero chance to ever be committed in our tree,
especially since this is in a md _types.h file, so this construct x10 times
in the tree.

>
>
> >
> >Â Â I'd rather not add new pre-defined macros if we can avoid it.
> There are
> >Â Â already too many. You can probably detect this situation
> with:
> >Â Â #if defined(_MSC_EXTENSIONS) && !defined(_WCHAR_T_DEFINED)
> >
> Was this added recently ?
> There is no _MSC_EXTENSIONS in the clang I'm using...
>
> Looks like we only define it for Windows targets.
> How about __is_identifier?
> $ cat t.cpp
> #if !__is_identifier(__wchar_t)
> #error "have __wchar_t"
> #else
> #error "no __wchar_t"
> #endif

Nope, that will trigger errors on non clang compilers.

You can wrap it in #ifdef __clang__ to fix that.

And that leads to more or less the same shitty set of conditionals I already
posted about... as opposed to using _MSC_EXTENSIONS, which by the way is
already defined on windows, is unlikely to ever collide with anything, and
would only be on with -fms-extensions, which is not the most common option
ever...

In every single case, __is_identifier will replace a one-liner
with several lines of nested conditionals... not a good thing.

Date: Fri, 12 May 2017 08:53:53 -0700
From: Reid Kleckner via llvm-dev <llvm-dev@lists.llvm.org>

Moving to cfe-dev to discuss clang things.

I think we already have _WCHAR_T_DEFINED for this:
  if (LangOpts.MicrosoftExt) {
    if (LangOpts.WChar) {
      // wchar_t supported as a keyword.
      Builder.defineMacro("_WCHAR_T_DEFINED");
      Builder.defineMacro("_NATIVE_WCHAR_T_DEFINED");
    }
  }

Unfortunately those aren't defined if you use -fms-extensions in C
mode as LangOpts.WChar is false in that case.

So, we actually have documentation on this, and __wchar_t is the example
use case:
https://clang.llvm.org/docs/LanguageExtensions.html#is-identifier

Please just use it, it works.

I do not want to define _MSC_EXTENSIONS more than we already do, and I
don't think we should add more pre-defined macros when we already have
these nicely factored feature test macros that don't pollute the global
namespace. The "simple solution" is not a good solution.

I believe our use of _MSC_EXTENSIONS is wrong anyway. It is controlled by
MSVC's /Ze and /Za flags, which do not relate to our -fms-extensions flag.

How about you actually read my problem ?

this is not what I'm trying to solve.

The error I get is:
/usr/include/machine/_types.h:132:15: error: cannot combine with previous 'int'
declaration specifier
      typedef int __wchar_t;

and it's *because* -fms-extensions does define __wchar_t.

A solution is nowhere as short as the fragment you quote from the documentation.

We reserve the right to provide __wchar_t in modes other than -fms-extensions in future, or to sometimes not provide it in that mode, so using _MSC_EXTENSIONS for this is simply a mistake.

If you want to detect whether we provide a builtin __wchar_t, __is_identifier is the mechanism we provide for that. It doesn’t make sense for us to add another.