<p dir="ltr">Ping</p>
<div class="gmail_quote">06 авг. 2013 г. 20:32 пользователь "Timur Iskhodzhanov" <<a href="mailto:timurrrr@google.com">timurrrr@google.com</a>> написал:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks for the comments!<br>
Please take a look at the updated patch.<br>
<br>
2013/8/5 John McCall <<a href="mailto:rjmccall@gmail.com">rjmccall@gmail.com</a>>:<br>
> On Aug 2, 2013, at 2:34 AM, Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com">timurrrr@google.com</a>> wrote:<br>
><br>
>  John,<br>
>  Any comments on this?<br>
><br>
><br>
> +llvm::Value *ItaniumCXXABI::BuildVirtualCall(CodeGenFunction &CGF,<br>
> +                                             GlobalDecl GD, llvm::Value<br>
> *This,<br>
> +                                             llvm::Type *Ty) {<br>
><br>
> I know you’re just using an existing name here, but the function is still<br>
> misnamed: it doesn’t actually build the call, it just gets the virtual<br>
> function<br>
> pointer.  So, maybe getVirtualFunctionPointer?<br>
<br>
Agreed, done.<br>
<br>
> +  llvm::Value *result =<br>
> +      CGF.Builder.CreateLoad(CGF.GetAddrOfLocalVar(getThisDecl(CGF)),<br>
> "this");<br>
> +<br>
> +  const CXXMethodDecl *MD = cast<CXXMethodDecl>(CGF.CurGD.getDecl());<br>
> +  if (MD->isVirtual()) {<br>
> +    result = CGM.getCXXABI()<br>
> +        .adjustThisParameterInVirtualFunctionPrologue(CGF, CGF.CurGD,<br>
> result);<br>
> +  }<br>
> +<br>
> +  getThisValue(CGF) = result;<br>
><br>
> The alloca should hold the correct value; that’s important for e.g. debug<br>
> info.<br>
> It’d be nice to do this during argument emission, but if necessary you can<br>
> just<br>
> store the adjusted value back into the alloca.<br>
<br>
Hm, I think I was able to put the correct value into alloca doing the<br>
adjustment in CodeGenFunction::EmitFunctionProlog(), please take a<br>
look if this approach is good enough?<br>
<br>
// At least this produces great LLVM code now!<br>
<br>
> +  unsigned AS =<br>
> cast<llvm::PointerType>(This->getType())->getAddressSpace();<br>
> +  llvm::Type *charPtrTy = CGF.Int8Ty->getPointerTo(AS),<br>
> +             *thisTy = This->getType();<br>
> +  if (ML.VBase) {<br>
> +    This = CGF.Builder.CreateBitCast(This, charPtrTy);<br>
> +    const ASTRecordLayout &DerivedLayout =<br>
> +        CGF.getContext().getASTRecordLayout(MD->getParent());<br>
> +    CharUnits VBaseOffset = DerivedLayout.getVBaseClassOffset(ML.VBase);<br>
> +    This = CGF.Builder.CreateConstGEP1_64(This,<br>
> -VBaseOffset.getQuantity());<br>
> +  }<br>
> +<br>
> +  CharUnits StaticOffset = ML.VFTableOffset;<br>
> +  if (!StaticOffset.isZero()) {<br>
> +    assert(StaticOffset.isPositive());<br>
> +    This = CGF.Builder.CreateBitCast(This, charPtrTy);<br>
> +    This = CGF.Builder.CreateConstGEP1_64(This,<br>
> -StaticOffset.getQuantity());<br>
> +  }<br>
><br>
> Just accumulate the constant offset locally and do one gep.<br>
<br>
Good point - done!<br>
<br>
> You should add a quick explanation of the algorithm here, covering why using<br>
> the vbase offset within the defining class is the right thing to do.  (It<br>
> doesn’t<br>
> need to be too long — just a sentence or two, then just refer to the<br>
> comments<br>
> in the vf-table layout code for a more complete explanation.)<br>
<br>
Done<br>
<br>
> Also, this will eventually need vtordisp logic, right?  Or is that done with<br>
> thunks?<br>
<br>
Yep, that's done with thunks.<br>
<br>
> +    /// VBase - If nonzero, holds the virtual base which holds this method<br>
> +    /// in its vftable.<br>
> +    const CXXRecordDecl *VBase;<br>
><br>
> “nonnull” instead of “nonzero”.<br>
><br>
> Also, the exact definition is that this is the virtual base subobject which<br>
> contains<br>
> the vf-table that the method definition is adjusted to.<br>
<br>
Done<br>
<br>
> +    /// Stores the method's "this" implicit argument type.<br>
> +    const CXXRecordDecl *ThisType;<br>
><br>
> You seem to be going through a lot of effort to propagate a type around<br>
> that,<br>
> frankly, isn’t even accurate when virtual bases come into play.  Consider<br>
> just<br>
> passing it as an i8*, at least in the vbase case.<br>
<br>
I assume you're referring to that "interesting" case when vbases got<br>
reordered and the "this" parameter was expected to point past the<br>
complete object?<br>
Yeah, I was considering using i8* from the beginning, but didn't<br>
completely come up with this special case yet...<br>
Done, now getThisArgumentTypeForMethod() is allowed to return 0 if it<br>
wants to indicate that we should use i8*.<br>
<br>
I kinda agree that calculating ThisType is a somewhat redundant step<br>
even if there is no virtual inheritance; I've put a FIXME for that<br>
just in case.<br>
<br>
I think my current solution (passing RD=0) is somewhat sloppy, but<br>
can't come up with a better alternative. Do you have any suggestions<br>
there?<br>
<br>
[I also considered making arrangeCXXMethodType() take MD rather than<br>
RD and making a CanQualType callback rather than RD-returning, but it<br>
turned out arrangeCXXMethodType() should actually work with "unknown<br>
C++ non-static member function of the given abstract type".]<br>
<br>
> John.<br>
</blockquote></div>