New issue
Advanced search Search tips

Issue 768624 link

Starred by 2 users
Status: Archived
Owner: ----
Closed: Jan 10
Cc:
thakis@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

We need to audit places that check defined(COMPILER_GCC) but not defined(__clang__)

Project Member Reported by brucedaw...@chromium.org, Sep 26 2017 
crrev.com/c/599127 fixed a bug where defining of PRINTF_FORMAT was wrapped with:

#if defined(COMPILER_GCC)

when it should have been wrapped with:

#if defined(COMPILER_GCC) || defined(__clang__)

This omission meant that format-string checking in clang was disabled on Windows (since, correct me if I'm wrong, clang defines COMPILER_GCC on Linux/OSX/Android but not on Windows).

A quick regex search for:

#if defined\(COMPILER_GCC\)\n

finds 16 places where COMPILER_GCC is checked without checking __clang__ as well. Some of these probably represent bugs. This one in particular looks suspicious.

// Macro for hinting that an expression is likely to be false.
#if !defined(UNLIKELY)
#if defined(COMPILER_GCC)
#define UNLIKELY(x) __builtin_expect(!!(x), 0)
#else
#define UNLIKELY(x) (x)
#endif  // defined(COMPILER_GCC)
#endif  // !defined(UNLIKELY)

These should be evaluated and fixed as appropriate. Adding an explicit __clang section and a #error section for unknown compilers would make it clearer which ones were wrong. The ones with a COMPILER_MSVC case confused me at first (it's set by clang-cl) and an explicit #else #error case would have clarified that.

Thoughts?

Comment 1 by thakis@chromium.org, Sep 26 2017 

Fixing __builtin_expect sounds good, and doing a quick audit sounds great too.

In general, the thinking was that many places will do

#if GCC
// do gcc thing
#elif MSVC
// do msvc thing
#endif

and as long as gcc thing and msvc thing are equivalent and clang supports both, we should be fine. If there's no equivalent msvc thing (as in the printf issue you fixed), that doesn't work of course :-)


And yes, clang identifies as gcc in gcc mode, and as cl in clang-cl mode. Slide 88 of https://docs.google.com/presentation/d/1oxNHaVjA9Gn_rTzX6HIpJHP7nXRua_0URXxxJ3oYRq0/edit#slide=id.gbf386ccf4_0_150 very quickly touches on this.

Comment 2 by benhenry@google.com, Jan 10

Status: Archived (was: Untriaged)
Archiving P3s older than 1 year with no owner or component.
►

Sign in to add a comment