Base declaration in pointer arithmetic expression is determined by
binary search with type information. Take "int *a, *b; *(a+*b)" as an
example, we determine the base by checking the type of LHS and RHS. In
this case the type of LHS is "int *", the type of RHS is "int",
therefore, we know that we need to visit LHS in order to find base
declaration.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 47224 Build 50061: arc lint + arc unit
Event Timeline
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15698 | Why should we do this in another patch? The only thing this patch does is extending for pointer arithmetic (doing analysis on BinOp). | |
15712 | Will it be better if just check if the subtree is an offset? So that we only need to check if it does not have any decorator in type? |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
9965 | I would say that this lvalue must be addressable, no? Also, some function calls can be handled, probably, those returning rvalues, for example, or constexprs. | |
clang/lib/CodeGen/CGOpenMPRuntime.cpp | ||
7616 | I think so. Otherwise those binary expressions are meaningless. Or treated in some unexpected way. | |
clang/lib/Sema/SemaOpenMP.cpp | ||
15698 | You're doing some checks in this patch using counting of '*' and '[' in this patch. This is definitely not correct. That's why I suggest to move it in a separate patch and discus the solution there and do not block all other kinds of expressions. | |
15712 | What do you mean? Exclude, say, RHS or LHS from the analysis, if it is a complex expression and you can use another part as a base? That's worth trying, at least. |
Remove the counting in MapBaseChecker, instead, comparing the type of root with
the type of LHS and RHS, and only visit the subtree having the same type as root.
This scheme pass all the test I have. I'll add BinOp into components in the next
revision. Thanks
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15712 | Generally yes, but the current implementation was the opposite way: visit the subtree with the same type as root (binop), if found, then we just visit it. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15683 | I think you need to extend this to support paren expressions, at least. | |
15708–15710 | I don't think you need these variables here anymore. | |
15714–15718 | This does not look correct. At least, it should return the result of one of Visit() calls. Plus, it seems to me, it won't handle something like *(a+5). That's why I suggest excluding binary op from this patch, it is not complete. |
clang/lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
7633 | I'm now not checking for isLValue here since the binary expression without unary operator is not an LValue. | |
clang/lib/Sema/SemaOpenMP.cpp | ||
15714–15718 | Now return the result of Visit() calls. Also, this patch can handle *(a+5) or more complicated pointer arithmetic expression, such as CK6 and CK13 in target_update_codegen.cpp. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15683 | I have UO->getSubExpr()->IgnoreParenImpCasts() here, or I might not understand what you mean by "support paren expression". |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15683 | (*(..)) is not supported, I think. Or, maybe, *((a)+b) |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15683 | Both cases are handled correctly. For (*((b)+a)), the binop in VisitBinaryOperator()does not include the outer paren and for *((a)+b), the parens can be handled by Expr *SubE = UO->getSubExpr()->IgnoreParenImpCasts();. |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
9965 | Fix this message, please, it is not quite correct. | |
clang/lib/CodeGen/CGOpenMPRuntime.cpp | ||
7616–7633 | Do you have the tests for these changes? | |
7751 | Change this line to just cast<FieldDecl>. In this case the assert is not required. | |
clang/lib/Sema/SemaOpenMP.cpp | ||
15683 | I think we need to change the way we check the expression in call for MapBaseChecker::Visit in checkMapClauseExpressionBase or extend the check in the checkMapClauseExpressionBase. E->IgnoreParenImpCasts() may drop implicit lvalue-to-rvalue cast and we may allow mapping of rvalues. | |
15695–15696 | I think, here we should have return RelevantExpr || Visit(UO->getSubExpr()) without dropping parens and implicit casts. Again, we may drop some important implicit casts | |
15703–15706 | Add a check here that the type of the expression must be a pointer type. | |
15715–15717 | Just return Visit(RE); without else | |
15718 | Unreachable code |
clang/test/OpenMP/target_update_codegen.cpp | ||
---|---|---|
371–372 | I suppose you missed some checks here, where IDX_EXT and TWO are used. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15690 | Hmm, addr_of creates rvalues. | |
15693 | No need for else here | |
15710 | Not sure this should be an assert. What if you have something like *((a+b) + c) and a+b is not pointer arithmetic? | |
15720 | No need for else here | |
clang/test/OpenMP/target_update_codegen.cpp | ||
374 | Must be just [[TWO]]? |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15710 | Well, for this case, the type of binop expr *((a+b) + c) is int *, and the type of LHS is int, while the type of RHS is int *, therefore, expr (a+b) is not going to be visited. I'm using assert here since before the checkMapClauseExpressionBase be called, we have already check for whether the expression is lvalue. Maybe I'll just error out if binop here is not a pointer type instead of assert? |
clang/test/OpenMP/target_update_codegen.cpp | ||
---|---|---|
374 | No, I mean that here you should not redefine the variable but check that the previously defined TWO is used in the check line. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15710 | Yes |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15684–15685 | UO_Deref is always r-value. So, !UO->isLValue() will be always triggered for UO_Deref. You can just exclude it from the condition. | |
clang/test/OpenMP/target_update_codegen.cpp | ||
375 | Seems to me, L is not used in the test, so you can just use {{%.+}} here |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15684–15685 | Oops, sorry, wrong comment, thought about Addr_of, but you removed it already. |
Also, would be good to add more tests for the situations that should not be handled, like just (a+b), explicit casts, etc.
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
9966 | I still would change this message to something like expected addressable lvalue in '%0' clause, where %0 must be one of map|to|from to be more specific and correct. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15499–15501 | Shall we do this only if RelevantExpr is nullptr? | |
15510–15511 | Shall we do this only if RelevantExpr is nullptr? | |
15570–15571 | Shall we do this only if RelevantExpr is nullptr? | |
15676–15680 | Shall we do this only if RelevantExpr is nullptr? | |
15707 | Shall we do this only if RelevantExpr is nullptr? | |
15712 | Shall we also here do return RelevantExpr || Visit(LE);? | |
15713 | Shall we also here do return RelevantExpr || Visit(RE);? |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15499–15501 | Yes, I'm assuming that RelevantExpr is nullptr before those assignments, so maybe adding assert is sufficient? |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15499–15501 | I think assert will crash for something like x?y:z. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15499–15501 | Though, currently, we just return false in this case. Yes, assert should be enough currently probably. |
I would say that this lvalue must be addressable, no? Also, some function calls can be handled, probably, those returning rvalues, for example, or constexprs.