Bug 17824: parse error in typedef union with -fms-extensions

Hi all,

I filed bug 17824 a few days ago because I'm seeing a regression in the
current svn clang vs previous versions with this structure in glibc's
/usr/include/stdlib.h when compiling with -fms-extensions:

/usr/include/stdlib.h:70:23: error: expected member name or ';' after declaration specifiers
    union wait *__uptr;
    ~~~~~ ^
1 error generated.

The definition in question is:

typedef union
  {
    union wait *__uptr;
    int *__iptr;
  } __WAIT_STATUS __attribute__ ((__transparent_union__));

and just trying to compile this on its own is enough to trigger the bug.
The __attribute__ ((__transparent_union__)) is not a part of the
problem; it can be removed without changing the error.

I thought I'd see if I can make some progress on this myself, but I'm
wondering where to start. I'm poking around in
tools/clang/lib/Parse/ParseDecl.cpp seeing if I can make any headway
here, but I'm wondering if there's some way to dump the (partial?) parse
tree or otherwise see how the parser is progressing?

Thanks,
    J

This is not actually a bug -- __uptr is a keyword in Microsoft mode.

http://msdn.microsoft.com/en-us/library/vstudio/aa983399.aspx

We don't support the full semantics for it, but you can see it
declared as a TypeAttr in Attr.td.

HTH!

~Aaron

Hi all,

I filed bug 17824 a few days ago because I'm seeing a regression in the
current svn clang vs previous versions with this structure in glibc's
/usr/include/stdlib.h when compiling with -fms-extensions:

This is not actually a bug -- __uptr is a keyword in Microsoft mode.

__sptr, __uptr | Microsoft Docs

We don't support the full semantics for it, but you can see it
declared as a TypeAttr in Attr.td.

Hi Aaron,

We've been good at supporting all the language standards built against stock system headers so far because it's handy for writing quick tests and, in this case, porting software from Windows.

The attached patch fixes -fms-extensions with GNU libc headers. Could you give it a spin against the MS headers?

Will land and get this merged to the release branch if it works for you.

Alp.

uptr-fix.patch (2.22 KB)

I have no issues with this patch, but I would wait for a LGTM from
someone like Richard. As for merging it into the release branch, that
should be discussed with Bill, and this seems reasonable to apply
there too.

Thanks!

~Aaron

  • // GNU libc++ uses certain MS keywords as identifiers.

“libc”, not “libc++”. Maybe specifically say that it uses __uptr, since that’s the only one we’re special-casing.

  • if (VendorAttributesAllowed && !getLangOpts().CPlusPlus &&
  • DS.isEmpty() && NextToken().is(tok::semi) &&
  • PP.getSourceManager().isInSystemHeader(Loc)) {

I’m worried about the generality of this check. An abstract-declarator could legitimately contain “T *__uptr”, followed by a semicolon. Can you pass in D.mayOmitIdentifier(), and treat __uptr as an identifier only if (1) the identifier cannot be omitted, and (2) the next token is a semicolon?

+ // GNU libc++ uses certain MS keywords as identifiers.

"libc", not "libc++". Maybe specifically say that it uses __uptr, since that's the only one we're special-casing.

+ if (VendorAttributesAllowed && !getLangOpts().CPlusPlus &&
+ DS.isEmpty() && NextToken().is(tok::semi) &&
+ PP.getSourceManager().isInSystemHeader(Loc)) {

I'm worried about the generality of this check. An abstract-declarator could legitimately contain "T *__uptr", followed by a semicolon.

Could you come up with an abstract declarator test case that actually ends in (keyword) __uptr followed by a semicolon in C mode? I couldn't.

Can you pass in D.mayOmitIdentifier(), and treat __uptr as an identifier only if (1) the identifier cannot be omitted, and (2) the next token is a semicolon?

Sure, let's do this to make the check more robust.

Alp.

+ // GNU libc++ uses certain MS keywords as identifiers.

"libc", not "libc++". Maybe specifically say that it uses __uptr, since
that's the only one we're special-casing.

+ if (VendorAttributesAllowed && !getLangOpts().CPlusPlus &&
+ DS.isEmpty() && NextToken().is(tok::semi) &&
+ PP.getSourceManager().isInSystemHeader(Loc)) {

I'm worried about the generality of this check. An abstract-declarator
could legitimately contain "T *__uptr", followed by a semicolon.

Could you come up with an abstract declarator test case that actually ends
in (keyword) __uptr followed by a semicolon in C mode? I couldn't.

I've looked through the C grammar and this appears to be impossible there.
There may be cases in Objective-C (or some other extension that we happen
to support) but I'm not aware of any.

Can you pass in D.mayOmitIdentifier(), and treat __uptr as an identifier

only if (1) the identifier cannot be omitted, and (2) the next token is a
semicolon?

Sure, let's do this to make the check more robust.

Thanks!

        + // GNU libc++ uses certain MS keywords as identifiers.

        "libc", not "libc++". Maybe specifically say that it uses
        __uptr, since that's the only one we're special-casing.

        + if (VendorAttributesAllowed && !getLangOpts().CPlusPlus &&
        + DS.isEmpty() && NextToken().is(tok::semi) &&
        + PP.getSourceManager().isInSystemHeader(Loc)) {

        I'm worried about the generality of this check. An
        abstract-declarator could legitimately contain "T *__uptr",
        followed by a semicolon.

    Could you come up with an abstract declarator test case that
    actually ends in (keyword) __uptr followed by a semicolon in C
    mode? I couldn't.

I've looked through the C grammar and this appears to be impossible there. There may be cases in Objective-C (or some other extension that we happen to support) but I'm not aware of any.

        Can you pass in D.mayOmitIdentifier(), and treat __uptr as an
        identifier only if (1) the identifier cannot be omitted, and
        (2) the next token is a semicolon?

    Sure, let's do this to make the check more robust.

Thanks!

Check added and landed in r195710!

Jeremy, this should work now. Thanks for reporting the issue

Alp.