This is an archive of the discontinued LLVM Phabricator instance.

Remove ObjectFile usage from HostLinux::GetProcessInfo
ClosedPublic

Authored by labath on Jan 24 2018, 8:40 AM.

Details

Summary

The ObjectFile class was used to determine the architecture of a running
process by inspecting it's main executable. There were two issues with
this:

  • it's in the wrong layer
  • the call can be very expensive (it can end up computing the crc of the whole file).

Since the process is running on the host, ideally we would be able to
just query the data straight from the OS like darwin does, but there
doesn't seem to be a reasonable way to do that. So, this fixes the
layering issue by using the llvm object library to inspect the file.
Since we know the process is already running on the host, we just need
to peek at a few bytes of the elf header to determine whether it's 32-
or 64-bit (which should make this faster as well).

Pretty much the same logic was implemented in
NativeProcessProtocol::ResolveProcessArchitecture, so I delete this
logic and replace calls with GetProcessInfo.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Jan 24 2018, 8:40 AM
clayborg accepted this revision.Jan 24 2018, 10:18 AM
clayborg added a subscriber: clayborg.

Looks fine to me.

This revision is now accepted and ready to land.Jan 24 2018, 10:18 AM
davide accepted this revision.Jan 24 2018, 10:24 AM
davide added a subscriber: davide.

Thanks for the spring cleaning!

eugene added inline comments.Jan 24 2018, 11:17 AM
source/Host/linux/Host.cpp
129 ↗(On Diff #131281)

Shouldn't it be

if (!buffer_sp)

?

labath planned changes to this revision.Jan 24 2018, 11:58 AM
labath added inline comments.
source/Host/linux/Host.cpp
129 ↗(On Diff #131281)

Indeed it should. Thanks.
I'll have to figure out tomorrow why the test still passed... :/

labath updated this revision to Diff 131401.Jan 25 2018, 2:04 AM

Ok, so the issue was a subsequent MergeFrom(HostArchitecture) call which was
overwriting the invalid ArchSpec I was returning here. I think this was present
there to fill in the bits we could not get from the ObjectFile (like some
environment details). Since now I am returning the ArchSpec I get from HostInfo,
the merge will become a no-op in the success case and produce garbage in case
the detection fails -- so I just remove the merge call.

This revision is now accepted and ready to land.Jan 25 2018, 2:04 AM
eugene accepted this revision.Jan 25 2018, 9:39 AM
clayborg accepted this revision.Jan 25 2018, 9:42 AM
This revision was automatically updated to reflect the committed changes.