This is an archive of the discontinued LLVM Phabricator instance.

[8.0 Regression] Fix handling of `__builtin_constant_p` inside template arguments, enumerators, case statements, and the enable_if attribute.
ClosedPublic

Authored by EricWF on Mar 6 2019, 12:15 PM.

Details

Summary

The following code is accepted by Clang 7 and prior but rejected by the upcoming 8 release and in trunk [1]

// error {{never produces a constant expression}}
void foo(const char* s) __attribute__((enable_if(__builtin_constant_p(*s) == false, "trap"))) {}
void test() { foo("abc"); }

Prior to Clang 8, the call to __builtin_constant_p was a constant expression returning false. Currently, it's not a valid constant expression.

The bug is caused because we failed to set InConstantContext when attempting to evaluate unevaluated constant expressions.

[1] https://godbolt.org/z/ksAjmq

Diff Detail

Event Timeline

EricWF created this revision.Mar 6 2019, 12:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2019, 12:15 PM
Herald added a subscriber: kristina. · View Herald Transcript
hans added a comment.Mar 7 2019, 3:09 AM

I think I'll have to defer to Richard for reviewing this, since I'm not familiar with the code. Since it's a regression we probably want to merge it to clang 8. Can you give any guidance to how bad a regression this is, e.g. does it break some important piece of code?

EricWF added a comment.Mar 7 2019, 3:45 AM

I think I'll have to defer to Richard for reviewing this, since I'm not familiar with the code. Since it's a regression we probably want to merge it to clang 8. Can you give any guidance to how bad a regression this is, e.g. does it break some important piece of code?

It breaks absl::StrFormat [1][2]. And it breaks libstdc++'s std::string_view inside unevaluated constant expressions [3].

This regression is a problem for Abseil.

[1] https://godbolt.org/z/Q_J2JP
[2] https://github.com/abseil/abseil-cpp/issues/271
[3 https://godbolt.org/z/Fg1sqM]

rsmith accepted this revision.Mar 8 2019, 10:00 AM

LGTM, and I think this is safe enough to take for Clang 8.

This revision is now accepted and ready to land.Mar 8 2019, 10:00 AM
hans added a comment.Mar 8 2019, 10:09 AM

LGTM, and I think this is safe enough to take for Clang 8.

Do you think the severity is high enough to spin another release candidate?

My concern is that since this didn't show up in testing until now, which I guess is quite a while after it regressed, maybe it's not important enough to delay the already late release? Also it sounds like it isn't completely fixed yet..

EricWF updated this revision to Diff 189888.Mar 8 2019, 11:05 AM
  • Fix regressions inside template parameters, case statements, and enumerators.

LGTM, and I think this is safe enough to take for Clang 8.

Do you think the severity is high enough to spin another release candidate?

Yes. Very much so. It also effect template arguments, case statements, enumerators, and probably more.

My concern is that since this didn't show up in testing until now, which I guess is quite a while after it regressed, maybe it's not important enough to delay the already late release?

The common case for hitting this bug requires using libstdc++ 7.0 or newer and C++17. This may be relatively rare for our release testers, who are probably either using libc++, targeting the system libstdc++, or not turning on C++17.
As libstdc++ 7.0 becomes more common and as more users move to C++17 people are going to run into this a lot.

Also it sounds like it isn't completely fixed yet..

It is now.

EricWF updated this revision to Diff 189927.Mar 8 2019, 2:01 PM

Add more tests.

EricWF updated this revision to Diff 189930.Mar 8 2019, 2:04 PM
EricWF retitled this revision from [8.0 Regression] Fix handling of `__builtin_constant_p` inside the enable_if attribute. to [8.0 Regression] Fix handling of `__builtin_constant_p` inside template arguments, enumerators, case statements, and the enable_if attribute..
  • Correct tests for unscoped enums.
This revision was automatically updated to reflect the committed changes.
rsmith added a comment.Mar 8 2019, 3:15 PM

LGTM, and I think this is safe enough to take for Clang 8.

Do you think the severity is high enough to spin another release candidate?

Given that the regression breaks both libstdc++ >=7 and abseil, I'm inclined to say yes. (Either that or we should aim to release a fixed 7.0.1 as soon as feasible after 7).