This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Extend builder to pass the TRIPLE spec to Make
ClosedPublic

Authored by JDevlieghere on Aug 7 2020, 10:19 AM.

Details

Summary

This is needed to build the test binaries for iOS. This is part of upstreaming the last missing pieces to run the on-device test suite with the upstream repository.

Diff Detail

Event Timeline

friss added a comment.Aug 7 2020, 10:43 AM

lldbremote.py looks awfully Apple-specific, yet it is going to be called from the generic code. Is that not an issue?

lldbremote.py looks awfully Apple-specific, yet it is going to be called from the generic code. Is that not an issue?

It's not an issue because it checks for the Apple SDK, but I could hoist that out of the helper to make it more obvious.

  • Simplify things a bit more
  • Add a comment explaining that the triple spec is only relevant with an Apple SDK

The part which bugs me about this is that the first thing that Makefile.rules will do with the triple we've painstakingly constructed here is it will decompose it into individual components, and then do some more matching on them, including running some xcrun commands, which are already being run here. I do think that doing this in python is a good idea (I believe the main reason a lot of this stuff is done in make is so that running "make" would just work, but we are way past that point now), but if we're going to do that in python, how about we move all of that to python? It seems the only effect of the TRIPLE argument is to add -target $(TRIPLE) -m$(OS)-version-min=$(VERSION) to CFLAGS. That should be easy to achieve using the information that construct_triple has available.

lldbremote.py looks awfully Apple-specific, yet it is going to be called from the generic code. Is that not an issue?

It's not an issue because it checks for the Apple SDK, but I could hoist that out of the helper to make it more obvious.

How about making an abstract/dummy getTripleSpec (or whatever) method in the base builder class. Then the darwin builder could override it do to its thing, and all of this code could reside in builder_darwin.py. If any other platform decides it needs to set the triple argument, then it's likely it will do that via some completely different method.

lldb/packages/Python/lldbsuite/test/lldbremote.py
37–39 ↗(On Diff #284010)

when dealing with triples, I believe this field is usually called the "os", not "platform".

The part which bugs me about this is that the first thing that Makefile.rules will do with the triple we've painstakingly constructed here is it will decompose it into individual components, and then do some more matching on them, including running some xcrun commands, which are already being run here. I do think that doing this in python is a good idea (I believe the main reason a lot of this stuff is done in make is so that running "make" would just work, but we are way past that point now), but if we're going to do that in python, how about we move all of that to python? It seems the only effect of the TRIPLE argument is to add -target $(TRIPLE) -m$(OS)-version-min=$(VERSION) to CFLAGS. That should be easy to achieve using the information that construct_triple has available.

Sounds reasonable.

lldbremote.py looks awfully Apple-specific, yet it is going to be called from the generic code. Is that not an issue?

It's not an issue because it checks for the Apple SDK, but I could hoist that out of the helper to make it more obvious.

How about making an abstract/dummy getTripleSpec (or whatever) method in the base builder class. Then the darwin builder could override it do to its thing, and all of this code could reside in builder_darwin.py. If any other platform decides it needs to set the triple argument, then it's likely it will do that via some completely different method.

Even though this file is called "base", there's no inheritance going on here, it's just methods and lldbtest.py will import the appropriate module. I considered this too, but didn't feel like rewriting those files as a classes for this small change. But if we're going to pass the TRIPLE, OS and VERSION separately, we might as well.

JDevlieghere planned changes to this revision.Aug 13 2020, 10:34 AM

Even though this file is called "base", there's no inheritance going on here, it's just methods and lldbtest.py will import the appropriate module. I considered this too, but didn't feel like rewriting those files as a classes for this small change. But if we're going to pass the TRIPLE, OS and VERSION separately, we might as well.

I'd just send the final ARCH_CFLAGS value. Then the Android.rules makefile could be ported to python as well.

Reimplement based on passing the ARCH_CFLAGS to Make

JDevlieghere added inline comments.Aug 18 2020, 5:06 PM
lldb/packages/Python/lldbsuite/builders/builder.py
224 ↗(On Diff #286431)

This should've been part of D86174, I'll fix that before updating the patch/landing.

jasonmolenda accepted this revision.Aug 19 2020, 12:51 AM

This looks good to me, getting any logic out of the Makefile templates that we can, that's a good improvement for maintainability.

This revision is now accepted and ready to land.Aug 19 2020, 12:51 AM
labath accepted this revision.Aug 19 2020, 5:17 AM
labath added inline comments.
lldb/packages/Python/lldbsuite/builders/builder.py
201–202 ↗(On Diff #286455)

Instead of a new method for each variable any subclass might want to set it would probably be better to just have an getExtraMakeArgs method that each subclass can return whatever it wants. ARCH_CFLAGS is fairly generic, so it may make sense to keep that separate, but DSYMUTIL and CODESIGN (and maybe SDKROOT) sound like they could go into the "extra" category.

Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2020, 11:47 AM