This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Unify and generalize Apple platforms in CMake and lit test configs
ClosedPublic

Authored by kubamracek on Jun 18 2018, 8:38 PM.

Details

Summary

There's more platforms than just "ios" and "iossim" that we should support, and adding more lit config variables for each platform isn't great. Let's generalize and have a single value that determines what the platform under test is.

Diff Detail

Repository
rL LLVM

Event Timeline

kubamracek created this revision.Jun 18 2018, 8:38 PM
Herald added subscribers: Restricted Project, mgorny. · View Herald TranscriptJun 18 2018, 8:38 PM
delcypher requested changes to this revision.Jun 19 2018, 2:38 AM

Overall looks very good. I have a few comments on some changes I'd like to see. The only one I think is essential is renaming the ios lit feature. However I think this could be done in a separate commit.

test/lit.common.cfg
111 ↗(On Diff #151845)

We should really fix the name of the ios lit configuration feature. The feature is added for any ios-like platform (i.e. Apple platform that is not macos) but it is really not obvious from the name. It could also be confused with config.apple_platform being set to ios which means something different. At some point in the future we might add the value config.apple_platform as a feature i.e.

config.available_features.add(config.apple_platform)

This would write tests that can be enabled/disabled based on config.apple_platform. However we can't do this if the ios feature means something else.

A possible new name for the ios feature could be apple_embedded_platform and the logic for setting this feature would simply be

if config.host_os == 'Darwin' and config.apple_platform != 'osx':
  config.available_features.add('apple_embedded_platform')

The feature name is a little long so I'd like to hear other ideas but its name should not match any of the possible values for config.apple_platform.

I realise this would be a very invasive change that would require changing lots of tests so if we do this I suggest this is done separately as a follow up commit.

119 ↗(On Diff #151845)

We could consider changing this to

"SANITIZER_" + config.apple.platform.upper() + "_TEST_DEVICE_IDENTIFIER"

I'm not sure if it's worth it. It would potentially be useful if in future if we wanted to test multiple devices in a single lit invocation.

120 ↗(On Diff #151845)

These scripts are named based on the old naming scheme and really should be fixed. E.g.

ios_run.py -> device_run.py
iossim_run.py -> device_sim_run.py

similarly for the _prepare, _env and _compile suffixed scripts too.

This would also allow you to rename the ios_or_iossim variable.

Side note the ios_commands directory also could do with a rename at some point.

130 ↗(On Diff #151845)

Should we have more error checking here?

132 ↗(On Diff #151845)

We should probably check that the JSON data is what we expect before trying to use it.

assert 'env' in prepare_output
assert isinstance(prepare_output['env'], dict)

Or something similar.

This revision now requires changes to proceed.Jun 19 2018, 2:38 AM

For the whole "ios/iossim" naming and ios_or_iossim: Let's not try to fix that in this patch. But in general, I don't think this is too terrible. The rule simply is: All embedded platforms claim they are also "ios" because they are derivatives of iOS. Maybe this could be greatly improved with just a bit more documentation? I like the fact that you can disable a test for all embedded platforms with just "DISABLE: ios" and you don't have to list all of them.

test/lit.common.cfg
130 ↗(On Diff #151845)

check_output will abort if the exit code isn' 0.

132 ↗(On Diff #151845)

Hm, this seems very non-Python-y. Just using prepare_output["env"] implicitly checks for these things as well.

Adding a comment explaining available features.

george.karpenkov accepted this revision.Jun 19 2018, 6:13 PM

Looks good to me, I think suggestions from @delcypher should be done in subsequent patches.

test/lit.common.cfg
141 ↗(On Diff #151918)

newline after colon

111 ↗(On Diff #151845)

@delcypher while I don't disagree, shouldn't this be done in another review?

120 ↗(On Diff #151845)

Again could be done in a subsequent review?

Adding a newline.

@delcypher Is there anything else you think needs to be addressed as part of this patch?

For the whole "ios/iossim" naming and ios_or_iossim: Let's not try to fix that in this patch. But in general, I don't think this is too terrible. The rule simply is: All embedded platforms claim they are also "ios" because they are derivatives of iOS. Maybe this could be greatly improved with just a bit more documentation? I like the fact that you can disable a test for all embedded platforms with just "DISABLE: ios" and you don't have to list all of them.

I really don't think using ios to mean ios and ios derivatives is a good convention to follow because it can be easily confused with ios being the config.apple_platform. What I proposed is to use a lit feature name like apple_embedded_platform (needs bikeshedding to be shorter) to mean any ios or ios derivative. With that you can still disable tests for all apple embedded platforms (i.e. replace UNSUPPORTED: ios with UNSUPPORTED: apple_embedded_platform in tests). With this change that then means we can use ios as the feature name to identify the ios platform (as opposed to watchos, tvos, etc.). In your current scheme you'd have to write something like UNSUPPORTED: ios && !(iossim || watchos || watchossim || tvos || tvossim) to mark a test as not support for the ios platform but okay for other platforms, which is very clumsy.

The fact that we use ios as a lit feature to mean ios or any ios derivative is just a historical holdover from ios being the only apple device platform we supported in testing. This isn't true anymore and I don't think adding some documentation to explain our (now odd ) convention is the right thing to do. It's simpler to just do the rename so that config.apple_platform == ios corresponds to the lit feature named ios.

I realise this would require quite a large change to the tests so I'm happy for this to change happen in a separate review.

test/lit.common.cfg
111 ↗(On Diff #151845)

I'm fine with that.

delcypher accepted this revision.Jun 20 2018, 2:08 AM
delcypher added inline comments.
test/lit.common.cfg
132 ↗(On Diff #151845)

Not quite. Python is duck typed so as long as prepare_output['env'] acts enough like a dict python is happy. You're right though that it probably isn't python-y, so I'll drop this point.

This revision is now accepted and ready to land.Jun 20 2018, 2:08 AM
This revision was automatically updated to reflect the committed changes.