Page MenuHomePhabricator

[LTO] Infer EKind/EMachine from Bitcode files
ClosedPublic

Authored by davide on Jun 27 2016, 7:03 PM.

Details

Summary

This is a proposed fix for https://llvm.org/bugs/show_bug.cgi?id=28268
I hope there's an easier way to get these informations because the current code is a little bit cumbersome :(

Diff Detail

Repository
rL LLVM

Event Timeline

davide updated this revision to Diff 62049.Jun 27 2016, 7:03 PM
davide retitled this revision from to [LTO] Infer EKind/EMachine from Bitcode files.
davide updated this object.
davide added a reviewer: rafael.
davide added a subscriber: llvm-commits.
ruiu added a subscriber: ruiu.Jun 27 2016, 7:24 PM
ruiu added inline comments.
ELF/InputFiles.cpp
691 ↗(On Diff #62049)

Instead of defining a special function for bitcode files, can you define getElfKind and getMachineType to BitcodeFile?

davide added inline comments.Jun 27 2016, 7:38 PM
ELF/InputFiles.cpp
691 ↗(On Diff #62049)

Sorry, maybe I'm misreading but you mean defining them as member functions of class BitcodeFile similarly to what happen in class ELFFileBase?
Or are you suggesting something else?

static ELFKind getELFKind();
const llvm::object::ELFFile<ELFT> &getObj() const { return ELFObj; }
llvm::object::ELFFile<ELFT> &getObj() { return ELFObj; }

uint16_t getEMachine() const { return getObj().getHeader()->e_machine; }
pcc added a subscriber: pcc.Jun 27 2016, 9:49 PM
pcc added inline comments.
ELF/InputFiles.cpp
707 ↗(On Diff #62049)

Maybe use getArch() and a switch statement instead of getArchName() here?

silvas added a subscriber: silvas.Jun 27 2016, 9:56 PM
silvas added inline comments.
ELF/InputFiles.cpp
708 ↗(On Diff #62049)

This seems really error-prone and likely to cause a long tail of bugs where we need to add stuff to the list (at the very least there should be a fatal error if we cannot infer). Think about it this way: ideally we will get the EMachine from the LTO object file later, but we need it earlier.
Can we somehow call into LLVM to get this information from MC? Maybe Rafael would know.

ruiu added a comment.Jun 27 2016, 10:51 PM

Can you please review http://reviews.llvm.org/D21784? This change should make it easier to do the same thing for bitcode files.

grimar added a subscriber: grimar.Jun 27 2016, 11:34 PM

I faced with close problem yesterday: a need of parcing the triples.
I ended up with the patch below. I do not think it is acceptable as final solution as it does
not check every bitcode file, but checks the LLD-INTERNAL-combined-lto-object result.
Though it works. Here just for completeness :)

D21786

grimar added inline comments.Jun 28 2016, 1:06 AM
ELF/InputFiles.cpp
699 ↗(On Diff #62049)

Seems you're not using the T.
What about

if (!TargetRegistry::lookupTarget(TripleStr, Msg))
  fatal("target not found: " + Msg);
703 ↗(On Diff #62049)

You're using IsLE and DL only once, I think this can be replaced with

if (M.getDataLayout().isLittleEndian())
mehdi_amini added inline comments.Jun 28 2016, 7:21 AM
ELF/InputFiles.cpp
694 ↗(On Diff #62049)

There is an API to get the Triple from a memory buffer without parsing the IR (I don't know if you need a module for something else here).

davide updated this revision to Diff 62174.Jun 28 2016, 8:55 PM

Rewrite after Rui's refactoring patch went in

Another possibility is to pass the triple to the function so that the computation of the triple from MemoryBuffer is shared in the constructor. I can change if you like that better.

ELF/InputFiles.cpp
723 ↗(On Diff #62174)

Used, thanks Mehdi!

736 ↗(On Diff #62174)

Done, thanks!

737 ↗(On Diff #62174)

Yes, unfortunately :( At least now we error out so we should able to catch those. I talked briefly with Rafael on IRC and he said we probably need a switch(), hopefully we can move away from it at some point.

ruiu added inline comments.Jun 28 2016, 9:57 PM
ELF/InputFiles.cpp
556 ↗(On Diff #62174)

M -> MB by convention.

556 ↗(On Diff #62174)

Can this be a non-member static function?

561–562 ↗(On Diff #62174)

Remove () surrounding Is64Bits.

565 ↗(On Diff #62174)

Ditto (M -> MB and non-member static function.)

568 ↗(On Diff #62174)

Can this be Triple(TripleStr).getArch())?

569–570 ↗(On Diff #62174)

Move this at end of this switch and inline fatal()

default:
  fatal("...");
592 ↗(On Diff #62174)

M -> MB

ruiu added inline comments.Jun 28 2016, 9:59 PM
ELF/InputFiles.cpp
589 ↗(On Diff #62174)

I'd include the triple to the error message.

fatal("unsupported architecture: " + TripleStr);
davide updated this revision to Diff 62178.Jun 28 2016, 10:09 PM

Rui's style comments addressed.

ruiu accepted this revision.Jun 28 2016, 10:14 PM
ruiu added a reviewer: ruiu.

LGTM with one more nit.

ELF/InputFiles.cpp
568 ↗(On Diff #62178)

Remove llvm:: from all cases.

This revision is now accepted and ready to land.Jun 28 2016, 10:14 PM
This revision was automatically updated to reflect the committed changes.
dsanders added inline comments.
lld/trunk/ELF/InputFiles.cpp
559–562

Sorry for the late comment but Triple::isArch64Bit() doesn't correspond to ELF64 or ELF32 on MIPS. In particular, the triple used for the N32 ABI will return true for isArch64Bit() but this ABI uses ELF32 objects. If lld doesn't support the N32 ABI yet then the current code is ok for the remaining two ABI's (O32 and N64).

davide added inline comments.
lld/trunk/ELF/InputFiles.cpp
559–562

+ Simon as he wrote the MIPS support.

> From: Rui Ueyama [ruiu@google.com]
> Sent: 06 July 2016 18:24
> To: reviews+D21779+public+dea87c23f5e22381@reviews.llvm.org
> Cc: Davide Italiano; Rafael Ávila de Espíndola; Daniel Sanders; George Rimar; Sean Silva; Peter Collingbourne; llvm-commits; Tim Amini Golling
> Subject: Re: [PATCH] D21779: [LTO] Infer EKind/EMachine from Bitcode files
>
> On Tue, Jul 5, 2016 at 6:33 AM, Daniel Sanders <daniel.sanders@imgtec.com<mailto:daniel.sanders@imgtec.com>> wrote:
> dsanders added a subscriber: dsanders.
>
> ================
> Comment at: lld/trunk/ELF/InputFiles.cpp:559-562
> @@ +558,6 @@
> + Triple TheTriple(TripleStr);
> + bool Is64Bits = TheTriple.isArch64Bit();
> + if (TheTriple.isLittleEndian())
> + return Is64Bits ? ELF64LEKind : ELF32LEKind;
> + return Is64Bits ? ELF64BEKind : ELF32BEKind;
> +}
> ----------------
> Sorry for the late comment but Triple::isArch64Bit() doesn't correspond to ELF64 or ELF32 on MIPS. In particular, the triple used for the
> N32 ABI will return true for isArch64Bit() but this ABI uses ELF32 objects. If lld doesn't support the N32 ABI yet then the current code is
> ok for the remaining two ABI's (O32 and N64).

Ah, interesting. So N32 is a 32-bit ABI on 64-bit MIPS just like x32 ABI is on x86-64. Do you know how to determine whether it is ELF32 or ELF64 for such triple?

In some ways, yes, but N32 has full support for 64-bit arithmetic. It was created to be similar to N64 but avoid the portability problems and bloat associated with 64-bit pointers.

It's not possible to distinguish N32 from N64 using the triple alone but I have patches under review to make this possible (D21465, D21467, D21069 for LLVM along with D21070, D21072 for clang). They're currently being discussed on llvm-dev in the 'Representing MIPS ABI information in the triple as ARM/X86 do for EABI/EABIHF/X32' thread. Assuming these patches are accepted, you'll be able to distinguish them using Triple::getEnvironment().


llvm-commits mailing list
llvm-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

> Ah, interesting. So N32 is a 32-bit ABI on 64-bit MIPS just like x32 ABI is on x86-64. Do you know how to determine whether it is ELF32 or ELF64 for such triple?
In some ways, yes, but N32 has full support for 64-bit arithmetic. It was created to be similar to N64 but avoid the portability problems and bloat associated with 64-bit pointers.

I've just noticed I've misread the table I was looking at. You're right, N32 serves the same purpose for 64-bit MIPS as X32 does for x86-64.

> On Thu, Jul 7, 2016 at 6:45 AM, Daniel Sanders <daniel.sanders@imgtec.com<mailto:daniel.sanders@imgtec.com>> wrote:
> dsanders added a comment.
> > > Ah, interesting. So N32 is a 32-bit ABI on 64-bit MIPS just like x32 ABI is on x86-64. Do you know how to determine whether it is ELF32 or ELF64 for such triple?
> > In some ways, yes, but N32 has full support for 64-bit arithmetic. It was created to be similar to N64 but avoid the portability problems and bloat associated with 64-bit pointers.
> I've just noticed I've misread the table I was looking at. You're right, N32 serves the same purpose for 64-bit MIPS as X32 does for x86-64.
Yup, x32 is not a popular ABI but it exists and different from i386 ABI. :) It made me wonder if we have the same issue for x32. Will your patch address the issue for x32 as well? (I'm just wondering because we don't support x32 in LLD yet.)

It doesn't look like X32 has the same problem. In fact, X86 seems to have solved the problem in almost the exact same way as my current patch series :-). The only difference being that it's adjusting the triple in the clang driver instead of cc1 and cc1as. The driver is probably the right place for it but I had trouble with the adjusted triple ending up in library paths when I tried it that way. I'll mention this in the other thread and hopefully that will unblock my patch series. I'll take another look at doing it in the driver while I'm waiting for people to reply.

You should be able to use 'Triple::getEnvironment() == Triple::GNUX32' to identify X32 since the current implementation computeTargetTriple() in the driver switches between Triple::GNUX32, Triple::GNU, and Triple::CODE16 to match the -m16/-m32/-mx32/-m64 options.