This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Simplify specifying of platform supported architectures
ClosedPublic

Authored by labath on Nov 10 2021, 12:56 PM.

Details

Summary

The GetSupportedArchitectureAtIndex pattern forces the use of
complicated patterns in both the implementations of the function and in
the various callers.

This patch creates a new method (GetSupportedArchitectures), which
returns a list (vector) of architectures. The
GetSupportedArchitectureAtIndex is kept in order to enable incremental
rollout. Base Platform class contains implementations of both of these
methods, using the other method as the source of truth. Platforms
without infinite stacks should implement at least one of them.

This patch also ports Linux, FreeBSD and NetBSD platforms to the new
API. A new helper function (CreateArchList) is added to simplify the
common task of creating a list of ArchSpecs with the same OS but
different architectures.

Diff Detail

Event Timeline

labath created this revision.Nov 10 2021, 12:56 PM
labath requested review of this revision.Nov 10 2021, 12:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2021, 12:56 PM
labath added inline comments.Nov 10 2021, 12:58 PM
lldb/include/lldb/Target/Platform.h
332

If all platforms (as I expect them to) implement this by maintaining a vector of supported architectures, this could be changed into an ArrayRef<ArchSpec>, but returning a value type for now simplifies the rollout.

JDevlieghere added inline comments.Nov 10 2021, 1:21 PM
lldb/include/lldb/Target/Platform.h
330–331

Do you expect any platform to implement both?

lldb/source/Target/Platform.cpp
1217–1218

In PlatformDarwin we set the vendor too. Could we have this take an optional OSType and Vendor maybe that sets the ones that are not None? That would also make migrating PlatformDarwin easier where we currently share the code and don't set the OS. Later we could move this out of PlatformDarwin into the child classes and have the OS set correctly.

labath added inline comments.Nov 11 2021, 7:45 AM
lldb/include/lldb/Target/Platform.h
330–331

Not really, though it wouldn't necessarily be an error if they did. I'll just remove the "at least" part.

lldb/source/Target/Platform.cpp
1217–1218

Umm.. maybe? I don't think this function is set in stone, but the nice thing about it being a helper function is that it's usage is completely optional. I haven't tried porting the darwin platforms yet, but, from a cursory glance I did see that they have a lot more complex usage patterns:

  • they pass subarchitectures as well, so we'd either also need to have an Optional subarch field, or switch to passing architectures as strings (I mean, I suppose I could switch to strings now, but it feels more correct to work with named constants)
  • a few of them also pass environments
  • and some don't even have matching OS fields (x86_64-apple-macosx and arm64-apple-ios in the same list)

So, I'd leave it to those patches to determine whether it's better to extend this function or create a new one.

labath updated this revision to Diff 386516.Nov 11 2021, 7:51 AM
  • remove "at least"
  • change CreateArchList argument order to match the triple format. This would be particularly important if new arguments are added later.
labath updated this revision to Diff 386543.Nov 11 2021, 9:25 AM

Fix a rather embarrasing off-by-one in the base implementation of
GetSupportedArchitectureAtIndex, which I guess also means I did not run tests
before uploading this (I have now).

JDevlieghere accepted this revision.Nov 11 2021, 9:31 PM

LGTM

lldb/source/Target/Platform.cpp
1217–1218

Okay, I'm happy to modify the helper when I start using it for the darwin platfomrs. I think at least some of the complexity/inconsistencies is purely historic such as the one where we were specifying an OS in the list of supported architectures in PlatformDarwin.

This revision is now accepted and ready to land.Nov 11 2021, 9:31 PM
labath marked 2 inline comments as done.Nov 16 2021, 2:41 AM
labath added inline comments.
lldb/source/Target/Platform.cpp
1217–1218

Cool. Does that mean I can count on you do convert the darwin platforms? :)

(I was going to try doing it myself, but I have basically no way of checking whether my modifications break anything.)

This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.
JDevlieghere added inline comments.Nov 16 2021, 1:16 PM
lldb/source/Target/Platform.cpp
1217–1218

Yup, although I can't commit to a timeframe :-) For validation you'd need to run the "on device" test suite and there's no real way for non-Apple people to do that, but if you get to it before me I'm more than happy to apply the patch locally and try it out.