Fix -Wimplicit-fallthrough warnings in regcomp.c

[Nathan-Huckleberry]

Summary

Since clang does not support comment style fallthrough annotations
these should be switched.

Compiler.h must be modified to allow c code to include it and allow LLVM_FALLTHROUGH to expand to attribute((fallthrough))

[nickdesaulniers]

Thanks for fixing the build!

[aaron.ballman]

llvm/lib/Support/regcomp.c

541

The macros in Compiler.h should be updated so that this can use LLVM_FALLTHROUGH instead to prevent compile errors.

[nickdesaulniers]

llvm/lib/Support/regcomp.c

541

ah, right, otherwise we get into situations where older versions of clang (or other bootstrap compilers) that don't have this feature don't understand this attribute

[aaron.ballman]

llvm/lib/Support/regcomp.c

541

Yes, as-is this would break everyone building Clang with MSVC (for instance).

[Nathan-Huckleberry]

llvm/lib/Support/regcomp.c

541

I'm having problems including Compiler.h from a c file. I can't include a cpp header into a c file.

In file included from llvm/llvm-project/llvm/lib/Support/regcomp.c:51:
llvm/llvm-project/llvm/include/llvm/Support/Compiler.h:19:10: fatal error: 'new' file not found
#include <new>
         ^~~~~

[aaron.ballman]

llvm/lib/Support/regcomp.c

541

You will have to add the appropriate conditionals to Compiler.h as part of the work. It seems it's already been done in some places, so it hopefully will not be too onerous. Adding an include to Compiler.h from a C file will help ensure we don't regress this functionality again in the future, which is a nice bonus.

[Nathan-Huckleberry]

• Add attribute to compiler.h

[xbolva00]

llvm/include/llvm/Support/Compiler.h

256–257

Move comment below?

575

#endif // __cplusplus

?

[aaron.ballman]

llvm/include/llvm/Support/Compiler.h

256–259

I think this may be better expressed as:

...
#elif __has_cpp_attribute(clang::fallthrough)
#define LLVM_FALLTHROUGH [[clang::fallthrough]]
#elif !defined(__cplusplus) && __has_attribute(fallthrough)
#define LLVM_FALLTHROUGH __attribute__((fallthrough))
#else
#define LLVM_FALLTHROUGH
#endif

[Nathan-Huckleberry]

• Code cleanup

[aaron.ballman]

LGTM aside from some minor nits.

llvm/include/llvm/Support/Compiler.h

19

#if defined(__cplusplus) please

530

#if defined(__cplusplus) here as well.

[nickdesaulniers]

llvm/include/llvm/Support/Compiler.h

530

is #ifdef __cplusplus against a style guide? Usually I only use defined if there's more than one symbol to check.

[Nathan-Huckleberry]

• Change if to ifdef

[aaron.ballman]

llvm/include/llvm/Support/Compiler.h

530

I don't think we care either way; I certainly don't have a preference here.

[rupprecht]

I think this patch is causing BB errors:
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/13605/steps/build%20stage1%20clang/logs/stdio

FAILED: lib/Support/CMakeFiles/LLVMSupport.dir/regcomp.c.o 
/usr/bin/cc  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/Support -I/b/sanitizer-x86_64-linux-bootstrap/build/llvm/lib/Support -I/usr/include/libxml2 -Iinclude -I/b/sanitizer-x86_64-linux-bootstrap/build/llvm/include -fPIC -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wno-missing-field-initializers -pedantic -Wno-long-long -Wno-comment -fdiagnostics-color -ffunction-sections -fdata-sections -O3    -UNDEBUG -MD -MT lib/Support/CMakeFiles/LLVMSupport.dir/regcomp.c.o -MF lib/Support/CMakeFiles/LLVMSupport.dir/regcomp.c.o.d -o lib/Support/CMakeFiles/LLVMSupport.dir/regcomp.c.o   -c /b/sanitizer-x86_64-linux-bootstrap/build/llvm/lib/Support/regcomp.c
In file included from /b/sanitizer-x86_64-linux-bootstrap/build/llvm/lib/Support/regcomp.c:51:0:
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/include/llvm/Support/Compiler.h:152:30: error: missing ')' after "__has_attribute"
 #if __has_cpp_attribute(clang::reinitializes)
                              ^
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/include/llvm/Support/Compiler.h:152:31: error:  ':' without preceding '?'
 #if __has_cpp_attribute(clang::reinitializes)
                               ^
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/include/llvm/Support/Compiler.h:255:30: error: missing ')' after "__has_attribute"
 #elif __has_cpp_attribute(gnu::fallthrough)
                              ^
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/include/llvm/Support/Compiler.h:255:31: error:  ':' without preceding '?'
 #elif __has_cpp_attribute(gnu::fallthrough)
                               ^
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/include/llvm/Support/Compiler.h:259:32: error: missing ')' after "__has_attribute"
 #elif __has_cpp_attribute(clang::fallthrough)
                                ^
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/include/llvm/Support/Compiler.h:259:33: error:  ':' without preceding '?'
 #elif __has_cpp_attribute(clang::fallthrough)
                                 ^
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/include/llvm/Support/Compiler.h:267:30: error: missing ')' after "__has_attribute"
 #if __has_cpp_attribute(clang::require_constant_initialization)
                              ^
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/include/llvm/Support/Compiler.h:267:31: error:  ':' without preceding '?'
 #if __has_cpp_attribute(clang::require_constant_initialization)

[nickdesaulniers]

llvm/trunk/include/llvm/Support/Compiler.h

19 ↗

(On Diff #216426)

The original code used #if __cplusplus (notice #if, not #ifdef). I'm surprised that worked. Maybe this change needs to as well?

[aaron.ballman]

llvm/trunk/include/llvm/Support/Compiler.h

19 ↗

(On Diff #216426)

That's not the issue causing the compile errors.

152 ↗

(On Diff #216426)

The problem is that __has_cpp_attribute doesn't exist in C mode, so the fallback definition from line 41 is used rather than the preprocessor implementation, and :: is a syntactically invalid token in C.

[nickdesaulniers]

llvm/trunk/include/llvm/Support/Compiler.h

255 ↗

(On Diff #216426)

then this should have a guard for __cplusplus, too then? (or an #if around the C++ parts).

[aaron.ballman]

llvm/trunk/include/llvm/Support/Compiler.h

255 ↗

(On Diff #216426)

Yes, I think this needs to become #elif defined(__cplusplus) && __has_cpp_attribute(gnu::fallthrough).

There may be a better way we can use macros to abstract that away, but my quick attempt failed: https://godbolt.org/z/f7I68U

[aaron.ballman]

While we discuss the fix, I went ahead and reverted in r369569 to get the bots back to green.

[ormris]

This comment was removed by ormris.

[Nathan-Huckleberry]

Maybe replace __has_cpp_attribute with something like this?

#ifndef has_cpp_attribute
#ifdef __cplusplus
# define has_cpp_attribute(x) __has_cpp_attribute(x)
#else
# define has_cpp_attribute(x) 0
#endif
#endif

[lebedev.ri]

I think the patch description doesn't actually describe the issue.

[lebedev.ri]

Is this only about lib/Support/regcomp.c? Can that source simply be switched to C++?

[aaron.ballman]

In D66487#1640019, @lebedev.ri wrote:

Is this only about lib/Support/regcomp.c? Can that source simply be switched to C++?

We have other C files that may wish to use attributes, so I don't see that as a long-term solution.

In D66487#1639946, @Nathan-Huckleberry wrote:

Maybe replace __has_cpp_attribute with something like this?

#ifndef has_cpp_attribute
#ifdef __cplusplus
# define has_cpp_attribute(x) __has_cpp_attribute(x)
#else
# define has_cpp_attribute(x) 0
#endif
#endif

That will run into the same issue -- you'll use has_cpp_attribute(clang::something) and in non-C++ mode, the :: will cause an error. You need to hide the :: behind the __cplusplus check.

[Nathan-Huckleberry]

In D66487#1639946, @Nathan-Huckleberry wrote:

Maybe replace __has_cpp_attribute with something like this?

#ifndef has_cpp_attribute
#ifdef __cplusplus
# define has_cpp_attribute(x) __has_cpp_attribute(x)
#else
# define has_cpp_attribute(x) 0
#endif
#endif

That will run into the same issue -- you'll use has_cpp_attribute(clang::something) and in non-C++ mode, the :: will cause an error. You need to hide the :: behind the __cplusplus check.

Shouldn't the preprocessor take care of that? has_cpp_attribute(clang::something) just gets replaced by 0? I tested locally and it appears to work.

[aaron.ballman]

In D66487#1640188, @Nathan-Huckleberry wrote:

In D66487#1639946, @Nathan-Huckleberry wrote:

Maybe replace __has_cpp_attribute with something like this?

#ifndef has_cpp_attribute
#ifdef __cplusplus
# define has_cpp_attribute(x) __has_cpp_attribute(x)
#else
# define has_cpp_attribute(x) 0
#endif
#endif

That will run into the same issue -- you'll use has_cpp_attribute(clang::something) and in non-C++ mode, the :: will cause an error. You need to hide the :: behind the __cplusplus check.

Shouldn't the preprocessor take care of that? has_cpp_attribute(clang::something) just gets replaced by 0? I tested locally and it appears to work.

I think I missed the lack of double underscores there -- I was worried about the Clang 3.6 behavior commented elsewhere in the file, but that won't happen here. You're right, this code should work. I would name it LLVM_HAS_BRACKET_ATTRIBUTE though to avoid confusion like what I ran into. Something like:

#ifndef LLVM_HAS_BRACKET_ATTRIBUTE
#if defined(__cplusplus) && defined(__has_cpp_attribute)
#define LLVM_HAS_BRACKET_ATTRIBUTE(x) __has_cpp_attribute(x)
#else
#define LLVM_HAS_BRACKET_ATTRIBUTE(x) 0
#endif
#endif

(I don't think we should name it LLVM_HAS_ATTRIBUTE because we also have __has_attribute checks for GNU-style attributes.)

[Nathan-Huckleberry]

New patch: https://reviews.llvm.org/D66609

llvm/include/llvm/Support/Compiler.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_COMPILER_H
 #define LLVM_SUPPORT_COMPILER_H
 
 #include "llvm/Config/llvm-config.h"
 
+#if __cplusplus
 #include <new>
+#endif
 #include <stddef.h>
 
 #if defined(_MSC_VER)
 #include <sal.h>
 #endif
 
 #ifndef __has_feature
 # define __has_feature(x) 0

 #else
 #define LLVM_ATTRIBUTE_RETURNS_NOALIAS
 #endif
 
 /// LLVM_FALLTHROUGH - Mark fallthrough cases in switch statements.
 #if __cplusplus > 201402L && __has_cpp_attribute(fallthrough)
 #define LLVM_FALLTHROUGH [[fallthrough]]
 #elif __has_cpp_attribute(gnu::fallthrough)
 #define LLVM_FALLTHROUGH [[gnu::fallthrough]]
-#elif !__cplusplus
-// Workaround for llvm.org/PR23435, since clang 3.6 and below emit a spurious
-// error when __has_cpp_attribute is given a scoped attribute in C mode.
-#define LLVM_FALLTHROUGH
+#elif !defined(__cplusplus) && __has_attribute(fallthrough)
+#define LLVM_FALLTHROUGH __attribute__((fallthrough))
 #elif __has_cpp_attribute(clang::fallthrough)
 #define LLVM_FALLTHROUGH [[clang::fallthrough]]
 #else
 #define LLVM_FALLTHROUGH
 #endif
 
 /// LLVM_REQUIRE_CONSTANT_INITIALIZATION - Apply this to globals to ensure that
 /// they are constant initialized.
 #if __has_cpp_attribute(clang::require_constant_initialization)

 #if __has_feature(cxx_exceptions)
 #define LLVM_ENABLE_EXCEPTIONS 1
 #elif defined(__GNUC__) && defined(__EXCEPTIONS)
 #define LLVM_ENABLE_EXCEPTIONS 1
 #elif defined(_MSC_VER) && defined(_CPPUNWIND)
 #define LLVM_ENABLE_EXCEPTIONS 1
 #endif
 
+#if __cplusplus
 namespace llvm {
 
 /// Allocate a buffer of memory with the given size and alignment.
 ///
 /// When the compiler supports aligned operator new, this will use it to to
 /// handle even over-aligned allocations.
 ///
 /// However, this doesn't make any attempt to leverage the fancier techniques
#ifdef __cpp_aligned_new
                     ,
                     std::align_val_t(Alignment)
 #endif
   );
 }
 
 } // End namespace llvm
 
+#endif // __cplusplus
 #endif

llvm/lib/Support/regcomp.c

 #include <limits.h>
 #include <stdlib.h>
 #include "regex_impl.h"
 
 #include "regutils.h"
 #include "regex2.h"
 
 #include "llvm/Config/config.h"
+#include "llvm/Support/Compiler.h"
 
 /* character-class table */
 static struct cclass {
   const char *name;
   const char *chars;
   const char *multis;
 } cclasses[] = {
   { "alnum",  "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz\
case '\\':
     } else {
       /* Other chars are simply themselves when escaped with a backslash.
        */
       ordinary(p, c);
     }
     break;
   case '{':   /* okay as ordinary except if digit follows */
     REQUIRE(!MORE() || !isdigit((uch)PEEK()), REG_BADRPT);
-    /* FALLTHROUGH */
+    LLVM_FALLTHROUGH;
   default:
     ordinary(p, c);
     break;
   }
 
   if (!MORE())
     return;
   c = PEEK();
if (p->pend[i] != 0) {
       (void) dupl(p, p->pbegin[i]+1, p->pend[i]);
       EMIT(O_BACK, i);
     } else
       SETERROR(REG_ESUBREG);
     p->g->backrefs = 1;
     break;
   case '*':
     REQUIRE(starordinary, REG_BADRPT);
-    /* FALLTHROUGH */
+    LLVM_FALLTHROUGH;
   default:
     ordinary(p, (char)c);
     break;
   }
 
   if (EAT('*')) {   /* implemented as +? */
     /* this case does not require the (y|) trick, noKLUDGE */
     INSERT(OPLUS_, pos);
case OCH_:
         s = *scan;
         /* assert() interferes w debug printouts */
         if (OP(s) != O_QUEST && OP(s) != O_CH &&
               OP(s) != OOR2) {
           g->iflags |= REGEX_BAD;
           return;
         }
       } while (OP(s) != O_QUEST && OP(s) != O_CH);
-      /* fallthrough */
+      LLVM_FALLTHROUGH;
     default:    /* things that break a sequence */
       if (newlen > g->mlen) {    /* ends one */
         start = newstart;
         g->mlen = newlen;
       }
       newlen = 0;
       break;
     }