Page MenuHomePhabricator

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

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



This improves upon, 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.


Diff Detail

Event Timeline

ahatanak created this revision.Apr 23 2020, 3:43 PM
rjmccall added inline comments.Apr 24 2020, 9:57 AM
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?

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.
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

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.


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.


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


Derived-to-base conversions.


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


Derived-to-base conversions.

ahatanak marked an inline comment as done.Tue, May 12, 12:07 PM
ahatanak added inline comments.

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

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

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.


You should add a case for unary *.


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.


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?


You should look through comma expressions.


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


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.


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.


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.


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

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



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.