Page MenuHomePhabricator

Added an assertion to constant evaluation enty points that prohibits dependent expressions
ClosedPublic

Authored by gribozavr on May 3 2019, 10:13 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

gribozavr created this revision.May 3 2019, 10:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2019, 10:13 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Can you add tests for the bugs you fixed? thanks

ABataev added inline comments.May 3 2019, 11:13 AM
clang/lib/Sema/SemaOpenMP.cpp
5784 ↗(On Diff #198042)

I would suggest to modify the code of this function if we cannot get the value of the loops.

if (CollapseLoopCountExpr->isValueDependent() || CollapseLoopCountExpr->isTypeDependent() || OrderedLoopCountExpr->isValueDependent() || OrderedLoopCountExpr->isTypeDependent()) {
  Built.clear(/* size */0);
  return 1;
}

at the beginning of the function.

Can you add tests for the bugs you fixed? thanks

The bugs were detected by existing tests (those tests triggered the newly added assertions).

gribozavr marked an inline comment as done.May 7 2019, 10:04 AM
gribozavr added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
5784 ↗(On Diff #198042)

I tried doing that, and a lot of tests started crashing with:

llvm-project/clang/lib/Sema/SemaOpenMP.cpp:9024: clang::StmtResult clang::Sema::ActOnOpenMPTargetTeamsDistributeSimdDirective(ArrayRef<clang::OMPClause *>, clang::Stmt *, clang::SourceLoc
ation, clang::SourceLocation, clang::Sema::VarsWithInheritedDSAType &): Assertion `(CurContext->isDependentContext() || B.builtAll()) && "omp target teams distribute simd loop exprs were not built"' failed.

Also, I wanted to note that if I were to make this change, then if EvaluateAsInt fails, we should apply the same recovery (Built.clear(); return).

ABataev added inline comments.May 7 2019, 10:06 AM
clang/lib/Sema/SemaOpenMP.cpp
5784 ↗(On Diff #198042)

The just try to use Built.clear(/* size */1);, I assume it must fix the problems.

rsmith added a comment.May 7 2019, 2:33 PM

The right thing to check in all of these cases should be only isValueDependent(). Every type-dependent expression should generally also be value-dependent (because the type is part of the value), but value-dependent exactly means "dependent in a way that prevents constant evaluation", so that's all that we should be checking.

gribozavr updated this revision to Diff 198603.May 8 2019, 2:22 AM

Addressed review comments:

  • Simplified error handling in SemaOpenMP,
  • Only check isValueDependent.
gribozavr marked an inline comment as done.May 8 2019, 2:23 AM

The right thing to check in all of these cases should be only isValueDependent(). Every type-dependent expression should generally also be value-dependent (because the type is part of the value), but value-dependent exactly means "dependent in a way that prevents constant evaluation", so that's all that we should be checking.

Done. PTAL.

clang/lib/Sema/SemaOpenMP.cpp
5784 ↗(On Diff #198042)

Yes, this works! Thanks!

rsmith added inline comments.May 8 2019, 4:53 PM
clang/lib/Sema/SemaOverload.cpp
6369 ↗(On Diff #198603)

This is treating value-dependent enable_if conditions as having failed. Is that really appropriate? (When do we call this with value-depnedent enable_if attributes? I'd expect it to only be called after substitution)

gribozavr marked an inline comment as done.May 9 2019, 7:23 AM
gribozavr added inline comments.
clang/lib/Sema/SemaOverload.cpp
6369 ↗(On Diff #198603)

This test case in llvm-project/clang/test/SemaCXX/enable_if.cpp passes a dependent condition:

void h(int);
template <typename T> void outer() {
  void local_function() __attribute__((enable_if(::h(T()), "")));
  local_function();
};

According to https://reviews.llvm.org/D20130, it seems like it was decided to document implementation details as specification, and say that enable_if is evaluated during overload resolution, whenever that happens to happen.

rsmith accepted this revision.May 17 2019, 1:20 AM
rsmith added inline comments.
clang/lib/Sema/SemaOverload.cpp
6369 ↗(On Diff #198603)

Hm. Yeah, I suppose we disable enable_if functions if the condition can't be evaluated, and this is a "can't be evaluated" case in some sense. I don't like it (the right thing would be to treat this as a dependent overload resolution), but as a short-term fix for this patch I suppose it's the right local choice.

This revision is now accepted and ready to land.May 17 2019, 2:04 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2019, 10:14 AM