Page MenuHomePhabricator

Fix LLVM C API for DataLayout
ClosedPublic

Authored by mehdi_amini on Aug 21 2015, 10:38 PM.

Details

Summary

We removed access to the DataLayout on the TargetMachine and
deprecated the C API function LLVMGetTargetMachineData() in r243114.
However the way I tried to be backward compatible was broken: I
changed the wrapper of the TargetMachine to be a structure that
includes the DataLayout as well. However the TargetMachine is also
wrapped by the ExecutionEngine, in the more classic way. A client
using the TargetMachine wrapped by the ExecutionEngine and trying
to get the DataLayout would break.

It seems tricky to solve the problem completely in the C API
implementation. This patch tries to address this backward
compatibility in a more lighter way in the C++ API. The C API is
restored in its original state and the removed C++ API is
reintroduced, but privately. The C API is friended to the
TargetMachine and should be the only consumer for this API.

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to Fix LLVM C API for DataLayout.
mehdi_amini updated this object.
mehdi_amini added a reviewer: echristo.
mehdi_amini added subscribers: llvm-commits, ributzka.

clang-format

So, you kept the old LLVMGetTargetMachineData working, but marked it as depreacated, and introduced the new LLVMGetDataLayout method as pa preferred way to get the datalayout. Is that it ?

While creating a module from the C API, one has to request the target machine for its datalayout to feed it to the module. How one is supposed to get this datalayout (in order to create a module) if the prefereed way to get a data layout require a module to begin with ?

Also, can we please have tests for the C API ? A good start would be D10725.

There is no API changes in this patch: LLVMGetTargetMachineData () was already deprecated, and I don't introduce LLVMGetDataLayout() it was already there.

This patch just fixes a bug that I introduced last month, in the way I tried to support this deprecated API with respect to the new C++ API. Because the current C++ API does not provide anymore a way to implement LLVMGetTargetMachineData, I had to hack around to keep it alive. Unfortunately it is broken with JIT clients.

I know you are interested in the C API, but the discussion about how to move forward on the C API policy is separated from this patch, ping me Monday and I'll try to make progress on this.

ributzka accepted this revision.Aug 24 2015, 3:19 PM
ributzka added a reviewer: ributzka.

LGTM

This revision is now accepted and ready to land.Aug 24 2015, 3:19 PM
echristo edited edge metadata.Aug 24 2015, 3:56 PM

This is pretty terrible and gets into maintenance of the C API questions. That said, if you want to keep the API around for a bit why not just create a new one every time?

-eric

mehdi_amini closed this revision.Sep 8 2015, 4:02 PM