Page MenuHomePhabricator

[Sema] Teach -Wcast-align to compute a more accurate alignment when the source expression has array subscript or pointer arithmetic operators
ClosedPublic

Authored by ahatanak on Apr 23 2020, 3:43 PM.

Details

Summary

This improves upon https://reviews.llvm.org/D21099, which taught -Wcast-align to look at the aligned attribute of variables. I added a function that computes a more accurate alignment to ExprConstant.cpp.

rdar://problem/59242343

Diff Detail

Event Timeline

ahatanak created this revision.Apr 23 2020, 3:43 PM
rjmccall added inline comments.Apr 24 2020, 9:57 AM
clang/include/clang/AST/Expr.h
710 ↗(On Diff #259739)

Nothing about the name here suggests that it only works on expressions of pointer type. Maybe call it getPresumedAlignmentOfPointer or something and then make it also consider the type alignment?

clang/lib/AST/ExprConstant.cpp
14844 ↗(On Diff #259739)

Does this do the right thing if getDeclAlign returns 0, or can that never happen?

ahatanak marked an inline comment as done.Apr 24 2020, 12:34 PM
ahatanak added inline comments.
clang/lib/AST/ExprConstant.cpp
14844 ↗(On Diff #259739)

I looked at getDeclAlign and, as far as I can tell, it returns an alignment that is larger than 0.

ahatanak planned changes to this revision.Apr 27 2020, 1:46 PM

The method has to compute the correct alignment for variables captured by lambdas or blocks too.

ahatanak updated this revision to Diff 263238.Mon, May 11, 12:35 PM
ahatanak marked an inline comment as done.

Define two helper functions in Sema that compute the alignment of a VarDecl and a constant offset from it and use them instead of the classes for evaluating lvalue and pointer expressions in ExprConstant.cpp.

Using the classes in ExprConstant.cpp to compute an expression alignment as I did in the previous patch is probably not a good idea since they are for evaluating constant expressions. They don't return the lvalue base variables and offsets in some cases ( for example, (A *)&m_ref in the test case) and using them for this purpose can make it harder to make changes to the way constant expressions are evaluated. The current patch is far from perfect as it misses many cases that could be detected by the classes in ExprConstant.cpp, but is at least an improvement over what we have now.

I was also thinking about fixing the alignment computation of captured variables, but I think I should do that separately in a follow-up as it might not be trivial. It will probably require looking up the captured variables in all the enclosing CapturingScopeInfos.

rjmccall added inline comments.Mon, May 11, 8:30 PM
clang/lib/Sema/SemaChecking.cpp
13161

I think an Optional<std::pair> would be a more self-documenting type here, and you'd be able to test the result in an if.

13177

You still need to conservatively adjust the alignment when the index isn't a constant expression. You can do that by reducing the base result to a simple alignment, adding the array element size as an offset, and then reducing again.

13186

Reference variables don't have to have initializers; e.g. they can be extern.

13206

Derived-to-base conversions.

13211

This all seems like it'd be simpler if you just checked for the pointer/integer operands and then swapped if necessary.

13215

Derived-to-base conversions.

ahatanak marked an inline comment as done.Tue, May 12, 12:07 PM
ahatanak added inline comments.
clang/lib/Sema/SemaChecking.cpp
13206

Does this include cases where bases are virtual? I think that would be more complex as it requires tracking the most-derived object. Do you think it's important to handle virtual bases in this patch or should it be done separately in a follow-up?

rjmccall added inline comments.Tue, May 12, 12:27 PM
clang/lib/Sema/SemaChecking.cpp
13206

You should handle virtual bases, but it's okay to assume they're not within known-complete objects; i.e. you just need to reset the alignment to the alignment of the base.

ahatanak updated this revision to Diff 263753.Wed, May 13, 9:33 AM
ahatanak marked 6 inline comments as done.

Address review comments. Handle derived-to-base cast expressions and array subscript expressions that don't have constant indices.

rjmccall added inline comments.Wed, May 13, 10:23 AM
clang/lib/Sema/SemaChecking.cpp
13122

Oh, this — and all the other places that do presumed alignment based on a pointee type — needs a special case for C++ records with virtual bases, because you need to get its presumed alignment as a base sub-object, not its presumed alignment as a complete object, which is what getTypeAlignInChars will return. The right way to merge that information is to get the normal alignment — which may be lower than expected if there's a typedef in play — and then min that with the base sub-object alignment.

13140

You should add a case for unary *.

13168

I don't think we guarantee that these will be no-op casts; all the explicit cast nodes should be handled the same as ImplicitCastExpr, in both of these functions.

13168

This should be handled the same as array subscripting. Maybe you can extract a helper for that that's given a pointer expression, an integer expression, and a flag indicating whether it's a subtraction?

13181

You should look through comma expressions.

13197

Can you do this with just a type analysis on the operands instead of eagerly doing these calls?

13211

There's an llvm::None which has this effect.

ahatanak updated this revision to Diff 264106.Thu, May 14, 2:50 PM
ahatanak marked 8 inline comments as done.

Address most of the review comments.

clang/lib/Sema/SemaChecking.cpp
13122

I think the base sub-object alignment in this case is the NonVirtualAlignment of the class (the CXXRecordDecl of Base in this function), but it's not clear to me what the normal alignment is. I don't think it's what Ctx.getTypeAlignInChars(Base->getType()) returns, is it? I see CodeGenModule::getDynamicOffsetAlignment seems to be doing what you are suggesting, but I'm not sure whether that's what we should be doing here. It looks like it's comparing the alignment of the derived class and the non-virtual alignment of the base class.

13168

The offset computation was wrong when the element type wasn't a char. The bug is fixed in the helper function.

Other changes look good to me, thanks.

clang/lib/Sema/SemaChecking.cpp
13122

The base sub-object alignment is the class's non-virtual alignment, right.

By the normal alignment, I mean the alignment you're starting from for the outer object — if that's less than the base's alignment, the base may be misaligned as well. For the non-base-conversion case, that's probably not necessary to consider.

ahatanak updated this revision to Diff 264157.Thu, May 14, 9:56 PM
ahatanak marked 2 inline comments as done.

Fix alignment computation of virtual bases.

ahatanak added inline comments.Thu, May 14, 9:57 PM
clang/lib/Sema/SemaChecking.cpp
13122

Let me know if the comment explaining why we are taking the minimum makes sense. I added a test case for this too (&t10).

rjmccall accepted this revision.Thu, May 14, 11:21 PM

LGTM.

clang/lib/Sema/SemaChecking.cpp
13122

Looks good.

This revision is now accepted and ready to land.Thu, May 14, 11:21 PM
This revision was automatically updated to reflect the committed changes.