[PATCH] D159339: [urgent][CodeGen] First check the kind and then the llvm::Function properties.

[David Tenty via Phabricator via cfe-commits]

daltenty accepted this revision.
daltenty added a comment.
This revision is now accepted and ready to land.

LGTM as a workaround.



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2389
   if (getTarget().getCXXABI().areMemberFunctionsAligned()) {
-    if (F->getPointerAlignment(getDataLayout()) < 2 && isa<CXXMethodDecl>(D))
+    if (isa<CXXMethodDecl>(D) && F->getPointerAlignment(getDataLayout()) < 2)
       F->setAlignment(std::max(llvm::Align(2), F->getAlign().valueOrOne()));
----------------
v.g.vassilev wrote:
> daltenty wrote:
> > Thanks for looking into this. 
> > 
> > It's not clear to me how this re-ordering ends up fixing things. Can you clarify what the uninitialized value was in this expression?
> > 
> > 
> The issue happens only in Release builds (RelWithDebInfo, too). From what I was able to see it is somewhere in `F->getPointerAlignment`. My assumption was that we cannot rely on the full properties of `F` to be set unless it is the declaration kind we expected (similar to checking if a something is a nullptr and then probing its members). Secondly the `isa` check is likely to be the less expensive check anyway.
> 
> I saw the issue yesterday and dug into it for a while. However, I decided to insert a "fix" before the release which is in few days since the `isa` seems to the faster check anyway.
Thanks, yeah thats kind of what I expected.  `F->getPointerAlignment()` is likely getting inlined into in to this callsite and we are inspecting uninitialized properties of the DataLayout. The weird part is I don't see why those properties of the DataLayout ever should be uninitialized, so I think there might be something more broken underneath this.

That said, this is definitely better than before as you say, so let's go ahead with this for the release and maybe I'll do some more digging in `getPointerAlignment`.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159339/new/

https://reviews.llvm.org/D159339