Determine whether the current clang version has a specific bug

I recently added the support to use attribute((unused)) on fields to silence -Wunused-private-field. Now, for backwards compatibility of new source files, we need a way to determine (e.g. in a header file) whether this is supported in the used clang version.

Currently __has_feature, __has_extension and __has_attribute implement something similar, but this change does not really fit any of those categories as it should be considered a bug. Not allowing this attribute on fields was an oversight that was fixed. Thus I (after discussion with chandlerc) suggest introducing __has_bug.

This should default to 1 (all bugs that are not explicitly fixed in a clang version are still bugs) and to provide compatibility with other compilers, sources can used:
#ifndef __has_bug

define __has_bug(x) clang

#endif

Any thoughts?
Daniel

I had the same idea (for some app’s plugin API, but the principle is the same). In this case, however, I think we should give the builtin a clang-specific name. __has_feature and __has_extension could be done the same way by other compilers with matching feature names, and code would profit. However, another compiler is unlikely to have exactly the same bug, or realize it and come up with the same name scheme (I would just use Bugzilla numbers). What’s more, since all unknown bug names are considered not fixed, that would mean that each such test would need a compiler predicate first. That is, if GCC also implemented this, then you can no longer just use the custom define you described, because __has_bug would already be defined. Then your code would think that GCC has every bug Clang ever had, and vice versa. So I think we should either call it __has_clang_bug, or else come up with a scheme to namespace the bug names and only claim that we have those that are in our namespace, i.e. __has_bug(clang, 12323) would default to true on Clang unless 12323 has been specifically fixed, whereas __has_bug(gcc, 65834) would default to false on Clang. The obvious downside of the second approach is that it is harder (maybe even impossible) to emulate using a macro. Sebastian

Currently __has_feature, __has_extension and __has_attribute implement something similar, but this change does not really fit any of those categories as it should be considered a bug. Not allowing this attribute on fields was an oversight that was fixed. Thus I (after discussion with chandlerc) suggest introducing __has_bug.

This should default to 1 (all bugs that are not explicitly fixed in a clang version are still bugs) and to provide compatibility with other compilers, sources can used:
#ifndef __has_bug

define __has_bug(x) clang

#endif

Any thoughts?

I had the same idea (for some app’s plugin API, but the principle is the same). In this case, however, I think we should give the builtin a clang-specific name. __has_feature and __has_extension could be done the same way by other compilers with matching feature names, and code would profit. However, another compiler is unlikely to have exactly the same bug, or realize it and come up with the same name scheme (I would just use Bugzilla numbers). What’s more, since all unknown bug names are considered not fixed, that would mean that each such test would need a compiler predicate first.
That is, if GCC also implemented this, then you can no longer just use the custom define you described, because __has_bug would already be defined. Then your code would think that GCC has every bug Clang ever had, and vice versa.

This is a great point.

So I think we should either call it __has_clang_bug, or else come up with a scheme to namespace the bug names and only claim that we have those that are in our namespace, i.e. __has_bug(clang, 12323) would default to true on Clang unless 12323 has been specifically fixed, whereas __has_bug(gcc, 65834) would default to false on Clang. The obvious downside of the second approach is that it is harder (maybe even impossible) to emulate using a macro.

I think __has_clang_bug(…) would work well.

We shouldn’t assume too much about the spelling of the ‘…’, I forsee at least two spelling patterns:

__has_clang_bug(cxx11_feature_we_messed_up)
__has_clang_bug(PR12345)

Isn't more lightweight to define only __clang_revision__ to SVN commit
id and to distribute with clang an header file where this revision is
mapped to fixed bugs in a way similar to this:

#define macrotest_1 ,
#define is_undef(macro) is_undef_(macro)
#define is_undef_(value) is_undef__(macrotest_##value)
#define is_undef__(comma) is_undef___(comma 0, 1)
#define is_undef___(_, v, ...) v

#define __clang_revision__ 123456

#if __clang_revision__ >= 123456
#define __clang_fixed_123 1
#endif

#define __has_clang_bug(id) is_undef(__clang_fixed_ ## id)

__has_clang_bug(123)
__has_clang_bug(124)

$ clang -E -P z.c
0
1

This would avoid to keep the whole clang bug history in compiler
executable and to add yet another extension. At the same time this
approach is extensible to whatever compiler (once someone is willing to
write its bug header file).

I'm actually very disturbed by the idea of __has_bug / __has_clang_bug because of this. There is no way to sync bug numbers up across compilers, especially if there's a bug in Clang that we fix in version X that always behaved correctly in GCC. The advantage of __has_feature and friends is that they're pessimistic -- if you get a 1, you know you can use the feature. Getting a 1 from __has_bug might just mean it's not a bug to begin with.

(What counts as a fixed bug? When we add a feature in SVN rXXX and then close a PR a month later when we notice it's been fixed, what's the right thing to do? What about regressions? Who is going to update the list when they fix a bug? Do our internal bugs count as bugs? Do our incremental fixes on longer projects count as bugs?)

The motivating use case is indeed motivating, since you get a warning if you do include [[unused]] on your private fields in old compilers (and in GCC), and a warning if you don't in new-Clang. And here __has_bug is being used pessimistically as well. But I don't think this is the way to solve the problem in general. Because __has_bug is vendor-specific, it's no better than comparing version numbers (trunk is not supposed to be stable) and probably not really more semantic. (If we came up with unique identifiers for the bugs it would be a little better, but that's more effort that I honestly don't think is necessary.)

For this one specific case, I'd rather extend __has_attribute to allow an optional context for the attribute. Another possibility would be to add __has_warning, but I'd be concerned that people would start using this to conditionally comment out code when compiling with certain warnings. (I haven't really thought that one through.)

That said, if we do decide to go with this, I agree with Abramo that it should be in a header file or at least an external table of some kind, not inside the Clang binary itself. (At least these feature checks aren't predefines, which would make every compilation pay for them.)

Jordan

__has_bug is a maintenance / code-bloat nightmare by design, and people should feel bad for proposing it. :slight_smile:

It would be totally reasonable to have a __has_feature(unused_attribute_on_fields), though.

John.

I had the same idea (for some app’s plugin API, but the principle is the same). In this case, however, I think we should give the builtin a clang-specific name. __has_feature and __has_extension could be done the same way by other compilers with matching feature names, and code would profit. However, another compiler is unlikely to have exactly the same bug, or realize it and come up with the same name scheme (I would just use Bugzilla numbers). What’s more, since all unknown bug names are considered not fixed, that would mean that each such test would need a compiler predicate first.

I’m actually very disturbed by the idea of __has_bug / __has_clang_bug because of this. There is no way to sync bug numbers up across compilers, especially if there’s a bug in Clang that we fix in version X that always behaved correctly in GCC. The advantage of __has_feature and friends is that they’re pessimistic – if you get a 1, you know you can use the feature. Getting a 1 from __has_bug might just mean it’s not a bug to begin with.

(What counts as a fixed bug? When we add a feature in SVN rXXX and then close a PR a month later when we notice it’s been fixed, what’s the right thing to do? What about regressions? Who is going to update the list when they fix a bug? Do our internal bugs count as bugs? Do our incremental fixes on longer projects count as bugs?)

The motivating use case is indeed motivating, since you get a warning if you do include [[unused]] on your private fields in old compilers (and in GCC), and a warning if you don’t in new-Clang. And here __has_bug is being used pessimistically as well. But I don’t think this is the way to solve the problem in general. Because __has_bug is vendor-specific, it’s no better than comparing version numbers (trunk is not supposed to be stable) and probably not really more semantic. (If we came up with unique identifiers for the bugs it would be a little better, but that’s more effort that I honestly don’t think is necessary.)

For this one specific case, I’d rather extend __has_attribute to allow an optional context for the attribute. Another possibility would be to add __has_warning, but I’d be concerned that people would start using this to conditionally comment out code when compiling with certain warnings. (I haven’t really thought that one through.)

__has_bug is a maintenance / code-bloat nightmare by design, and people should feel bad for proposing it. :slight_smile:

I won’t tell, who first proposed it ;-).

It would be totally reasonable to have a __has_feature(unused_attribute_on_fields), though.

Implemented in the attached patch, kindly asking for a review. I have chosen __has_feature(attribute_unused_on_fields) to better fit with the others.

Cheers,

Daniel

has_feature.patch (1.12 KB)

Can someone please review this patch?

Thank you!
Daniel

has_feature.patch (1.12 KB)

The __has_feature looks fine, but shouldn't you at least put an example in the test case?

+class HasFeatureTest {
+#if __has_feature(attribute_unused_on_fields)
+ int unused_; // expected-warning{{private field 'unused_' is not used}}
+ int unused2_ __attribute__((unused)); // no-warning
+#endif
+};

Looks fine to me.

John.

Changed and submitted as r159252.