[cfe-dev] [cfe-commits] [PATCH] Add PNaCl ABIInfo

John McCall rjmccall at apple.com
Fri Sep 23 11:33:58 PDT 2011


Taking this back to cfe-dev, since it's not really patch review anymore.

On Sep 23, 2011, at 1:23 AM, David Meyer wrote:
>> Correct me if I'm wrong, but this "ABI" is a recent invention of yours that
>> does not currently affect the binary compatibility of any released software.
>> It is apparently specified directly in terms of what is otherwise an
>> internal implementation detail of Clang — namely, the IR type that we
>> translate a struct into.  As we've brought up many times already, this
>> translation is very lossy and will not be adequate for your purposes
>> without significant embellishments of the LLVM type system which
>> many core developers would not be comfortable with.
> 
> How Clang translates a struct to IR is not an internal implementation
> detail of Clang. It is very much driven by the target ABI. In many
> cases, you have no choice. The platform ABI forces you to lower
> structs in a certain way.

Other platform ABIs are not specified in terms of LLVM IR types.  This
is very much part of my objection to what you're doing.

Most platform ABIs just care that we emit code that gives the overall
object a specific size and minimum alignment and that places various
sub-objects at specific byte offsets within the object.  In Clang, we can
make that happen in whatever way we please.  For example, we can
emit structs as [N x i8], use explicit alignments everywhere, and use
bytewise GEPs and bitcasts to get this result.  We certainly *try* not to
do that, but there are situations with unions and C++ class layout
where we don't always have a more satisfying choice.

>> In short, several of us are objecting to your choice of ABI, which we do
>> have the right to do, since that ABI is intrinsically intended to be a
>> permanent constraint on LLVM and Clang.
> 
> I have not seen an objection from anybody else besides you, and Eli
> has already approved this change. What is the procedure for resolving
> this kind of dispute?

I seem to remember Eli saying that the patch was okay, with some minor
tweaks, if these discussions decided that the technical direction was okay.
And I haven't really looked at the patch yet, but I think that's still the right
conclusion.

>> So, first off, your current method doesn't satisfy all of your requirements,
>> because it's not capable of being correctly translated into
>> target-dependent IR.  So there is really no reason to privilege it.
> 
> As I mentioned in the other thread, we don't require full ABI
> compatibility, only partial ABI compatibility. We're reasonably
> confident we can meet these requirements with minor tweaks to the
> current representation.

Part of what I'm concerned about is this patch being followed up with an
endless series of requests to change the IR translations of various structs
and unions to fix critical miscompiles in your code.

> It's true we have to do this stack allocation anyway in the IR
> generated by the frontend, but at least when it is done there, it will
> be optimized out if it is not needed.

There is no reason it can't also be optimized out after doing your
target-specific lowering.  Generic optimizations already do this kind of
thing all the time.

> Even though we could switch to byval, we really need a better argument
> for making such a shift, beyond just minimizing the patch to Clang.

>From my perspective, adding complexity to Clang needs a better argument
than "we'd rather not revisit our previous decisions if it implies any extra
work for us".

>>> While our ABI is not 100% finalized, until this patch is committed, we
>>> can't do any external testing with Clang. (unless we create our own
>>> branch, which we'd rather not)
>> 
>> I'm sorry, but we do expect code to go through code review, which
>> includes a thorough review of the underlying motivations and technical
>> design.
> 
> This doesn't make sense. Ultimately, the PNaCl project controls its
> own ABI. And I don't mean me. I was not involved in the decision to
> use first-class struct arguments, and if that decision needs to be
> altered, I'm not the person who has the final say on it.

The use of FCAs is really far less important than the extremely
cavalier approach towards ABI compliance.  I want that problem to be
solved well, not turned into an endless set of bugs because you kept
punting it and assuming it would work out until it suddenly started to
matter.  And I would like it to be solved before you actually release
this and turn an off-hand decision into a permanent constraint.

John.



More information about the cfe-dev mailing list