mstorsjo added a comment.

In D157332#4568341 <https://reviews.llvm.org/D157332#4568341>, @efriedma wrote:

> I see what you're getting at here... but I don't think this works quite 
> right.  If the empty class has a non-trivial constructor, we have to pass the 
> correct "this" address to that constructor.  Usually a constructor for an 
> empty class won't do anything with that address, but it could theoretically 
> do something with it.

Hmm, I think I see. In this specific case, there's no constructor for the empty 
class invoked at all, we call the function `f()` which returns an `struct S` 
(which is empty). But if we'd remove the initialization of `y(f())` and add a 
constructor to `S()`, I see that we generate code that is indeed wrong in that 
aspect.

> In order to preserve the address in the cases we need it, we need a different 
> invariant: the handling for each aggregate expression in EmitAggExpr needs to 
> ensure it doesn't store anything to an empty class.  Which is unfortunately 
> more fragile than I'd like, but I can't think of a better approach.  This 
> check needs to happen further down the call stack.  Maybe put it in 
> CodeGenFunction::EmitCall.

Ok, I'll see if I can look further into that... (I don't have a huge amount of 
time for it at the moment, so if someone else with an interest in the area, 
e.g. @crtrott or @amyk want to dig into it, feel free!)



================
Comment at: clang/test/CodeGenCXX/ctor-empty-nounique.cpp:1
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+
----------------
cor3ntin wrote:
> This should probably get another run line for powerpc64le
Sure, I could do that. (Initially I had both, but thought people would consider 
it unnecessary duplication.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to