[cfe-commits] [PATCH] Add PNaCl ABIInfo

Clang Frontend
1

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

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.

2

John,

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.

The PNaCl platform *is* IR, so we have no choice but to define our ABI
in terms of what the IR looks like.

The native platform ABI is an implementation detail which is hidden
behind llc, which runs inside Chromium and is under our control.

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.

You are forced to generate IR which will make the backend generate
calls conforming to the native ABI.
This is a leaky abstraction, since it requires the frontend to know
about how the backend lowers IR arguments to native code.
The fact that there are multiple ways to do it is hardly a useful feature.

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.

We have a very good idea of the ABI we want to have. FCA have worked
well for us, and we are happy to continue using them. You are the one
who is calling into question our ABI decision.

Regarding byval, for the sake of argument, we tried it your way, to
see what would happen.

We discover that ARM's support of byval is shaky at best. We've
already discovered three major bugs. Since the front-ends usually
don't general byval on ARM, it is not considered supported. They have
taken steps towards removing support for it entirely.

What are we suppose to do about that? We don't have the luxury of
emitting different IR for each target backend.

- pdox

3

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.

The PNaCl platform *is* IR, so we have no choice but to define our ABI
in terms of what the IR looks like.

I understand. I don't really have a good answer for you. My worry is
that (1) you're assuming a lot about how frontends translate C/C++
struct types into IR and (2) that translation isn't injective and will never
be able to be perfectly reversed.

I can tolerate an ABI dependence on Clang's translation of types into IR
if you are willing to promise me two things:
  A) You only really care about the translation of a fixed and small
    number of types.
  B) You have looked at how all of those types are translated into IR,
    and the translation is perfect; they are all translated into LLVM struct
    types with exactly the right members.

If you can only promise A, then we can work on B and try to get it right.
If you can't promise A, then I think you have a problem which needs to
be solved properly before you make promises we can't keep, and I'm
only sorry that it's not going to be an easy problem.

You are forced to generate IR which will make the backend generate
calls conforming to the native ABI.
This is a leaky abstraction, since it requires the frontend to know
about how the backend lowers IR arguments to native code.
The fact that there are multiple ways to do it is hardly a useful feature.

Yeah. Again, I'm not really defending the current setup; I'd be much
happier with a system where we could pass structs "by value" in a
high-level way, only special-casing truly special things like transparent
unions and C++ classes with nontrivial copy constructors. But that's
not what we have, and we're a lot of design away from anything like it.

Regarding byval, for the sake of argument, we tried it your way, to
see what would happen.

We discover that ARM's support of byval is shaky at best. We've
already discovered three major bugs. Since the front-ends usually
don't general byval on ARM, it is not considered supported. They have
taken steps towards removing support for it entirely.

What are we suppose to do about that? We don't have the luxury of
emitting different IR for each target backend.

I think there might have been a miscommunication. You already have
a pass that rewrites call sites and function declarations, right?
Something that runs before the backend ever sees IR, breaking up
aggregates, introducing byval slots, etc. as required by the host
platform's ABI? I was just suggesting that you teach that code to
work on byval arguments and parameters instead of FCA arguments
and parameters. If anything, that's probably less work, because hey,
sometimes you really do want a byval argument.

John.