Page MenuHomePhabricator

Fix -Wimplicit-fallthrough warnings in regcomp.c
ClosedPublic

Authored by Nathan-Huckleberry on Aug 20 2019, 11:18 AM.

Details

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))

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2019, 11:18 AM
nickdesaulniers accepted this revision.Aug 20 2019, 11:28 AM

Thanks for fixing the build!

This revision is now accepted and ready to land.Aug 20 2019, 11:28 AM
aaron.ballman requested changes to this revision.Aug 20 2019, 11:32 AM
aaron.ballman added inline comments.
llvm/lib/Support/regcomp.c
540 ↗(On Diff #216196)

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

This revision now requires changes to proceed.Aug 20 2019, 11:32 AM
nickdesaulniers requested changes to this revision.Aug 20 2019, 11:55 AM
nickdesaulniers added inline comments.
llvm/lib/Support/regcomp.c
540 ↗(On Diff #216196)

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 added inline comments.Aug 20 2019, 12:29 PM
llvm/lib/Support/regcomp.c
540 ↗(On Diff #216196)

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

Nathan-Huckleberry marked an inline comment as done.Aug 20 2019, 12:33 PM
Nathan-Huckleberry added inline comments.
llvm/lib/Support/regcomp.c
540 ↗(On Diff #216196)

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>
         ^~~~~
Nathan-Huckleberry marked an inline comment as not done.Aug 20 2019, 12:37 PM
aaron.ballman added inline comments.Aug 20 2019, 12:41 PM
llvm/lib/Support/regcomp.c
540 ↗(On Diff #216196)

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.

  • Add attribute to compiler.h
xbolva00 added inline comments.
llvm/include/llvm/Support/Compiler.h
258 ↗(On Diff #216221)

Move comment below?

580 ↗(On Diff #216221)

#endif // __cplusplus

?

aaron.ballman added inline comments.Aug 20 2019, 1:29 PM
llvm/include/llvm/Support/Compiler.h
257–264 ↗(On Diff #216221)

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 marked 3 inline comments as done.Aug 20 2019, 2:02 PM
xbolva00 accepted this revision.Aug 20 2019, 2:11 PM
aaron.ballman accepted this revision.Aug 20 2019, 2:18 PM

LGTM aside from some minor nits.

llvm/include/llvm/Support/Compiler.h
19 ↗(On Diff #216245)

#if defined(__cplusplus) please

530 ↗(On Diff #216245)

#if defined(__cplusplus) here as well.

nickdesaulniers accepted this revision.Aug 20 2019, 2:45 PM
nickdesaulniers added inline comments.
llvm/include/llvm/Support/Compiler.h
530 ↗(On Diff #216245)

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

This revision is now accepted and ready to land.Aug 20 2019, 2:45 PM
  • Change if to ifdef
aaron.ballman accepted this revision.Aug 20 2019, 3:37 PM
aaron.ballman added inline comments.
llvm/include/llvm/Support/Compiler.h
530 ↗(On Diff #216245)

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

This revision was automatically updated to reflect the committed changes.

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)
llvm/trunk/include/llvm/Support/Compiler.h
19

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

aaron.ballman added inline comments.Aug 21 2019, 12:10 PM
llvm/trunk/include/llvm/Support/Compiler.h
19

That's not the issue causing the compile errors.

152

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.

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

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

aaron.ballman added inline comments.Aug 21 2019, 12:46 PM
llvm/trunk/include/llvm/Support/Compiler.h
255

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

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

ormris added a subscriber: ormris.Aug 21 2019, 1:10 PM
This comment was removed by ormris.

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

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

Nathan-Huckleberry edited the summary of this revision. (Show Details)Aug 21 2019, 1:44 PM

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

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.

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.

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.

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.)