Page MenuHomePhabricator

[clang] avoid strict aliasing violation in assert
Needs ReviewPublic

Authored by rlibby on Dec 22 2019, 11:29 PM.



StoredDeclsList::setOnlyValue wants to assert that the binary value
actually stored in the member PointerUnion Data is the same as the raw
NamedDecl pointer argument. However the way in which it was checking
this involved a cast which created a type-punned pointer, and
dereferencing that pointer broke strict aliasing rules.

I have a build of clang by gcc where gcc emitted code to read from the
pointer for the assert before writing to it and so caused the assertion
to fail.

I'm not sure this is the best alternative formulation for the assert.
Please feel free to suggest another spelling as appropriate.

Event Timeline

rlibby created this revision.Dec 22 2019, 11:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2019, 11:29 PM
rlibby added a subscriber: dim.Jan 8 2020, 9:45 AM
dim added inline comments.Jan 9 2020, 11:00 AM

Hm, I don't have much experience with the PointerUnion class, but from a cursory look at llvm/include/llvm/ADT/PointerUnion.h, I would rather say that using dyn_cast here would be more appropriate? E.g. that definition literally says:

/// Returns the current pointer if it is of the specified pointer type,
/// otherwises returns null.

So something like:

assert(Data.dyn_cast<NameDecl> == ND &&
  "PointerUnion mangles the NamedDecl pointer!");

or even using the existing getAsDecl member function, which does exactly the same:

assert(Data.getAsDecl() == ND &&
  "PointerUnion mangles the NamedDecl pointer!");

Or is this particular check about the type being *only* the base class NamedDecl, and not any derived class?

I am no expert here so I will defer, but I believe those suggestions are weaker assertions.

I believe it's really trying to assert that the NamedDecl type is the first template type in the point union, and is represented by a 0 bit in the addr, and that the pointer was appropriately aligned. So I tried to
choose something that would fail if those were not true. I think that the dyn_cast<> approach will not fail appropriately if the NamedDecl type is not first.