[PATCH] D48309: [sanitizer] Unify and generalize Apple platforms in CMake and lit test configs

[Dan Liew via Phabricator via llvm-commits]

delcypher added a comment.

In https://reviews.llvm.org/D48309#1136545, @kubamracek wrote:

> 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.



================
Comment at: test/lit.common.cfg:111
+
   config.available_features.add('ios')
+  if ios_or_iossim == "iossim":
----------------
george.karpenkov wrote:
> delcypher wrote:
> > 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.
> @delcypher while I don't disagree, shouldn't this be done in another review?
I'm fine with that.


https://reviews.llvm.org/D48309