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

[Dan Liew via Phabricator via llvm-commits]

delcypher requested changes to this revision.
delcypher added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: test/lit.common.cfg:111
+
   config.available_features.add('ios')
+  if ios_or_iossim == "iossim":
----------------
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.


================
Comment at: test/lit.common.cfg:119
+
+  device_id_env = "SANITIZER_" + ios_or_iossim.upper() + "_TEST_DEVICE_IDENTIFIER"
+  run_wrapper = os.path.join(ios_commands_dir, ios_or_iossim + "_run.py")
----------------
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.


================
Comment at: test/lit.common.cfg:120
+  device_id_env = "SANITIZER_" + ios_or_iossim.upper() + "_TEST_DEVICE_IDENTIFIER"
+  run_wrapper = os.path.join(ios_commands_dir, ios_or_iossim + "_run.py")
+  env_wrapper = os.path.join(ios_commands_dir, ios_or_iossim + "_env.py")
----------------
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.


================
Comment at: test/lit.common.cfg:130
+
+  prepare_output_json = subprocess.check_output([prepare_script, config.apple_platform, config.clang]).strip().split("\n")[-1]
+  prepare_output = json.loads(prepare_output_json)
----------------
Should we have more error checking here?


================
Comment at: test/lit.common.cfg:132
+  prepare_output = json.loads(prepare_output_json)
+  config.environment.update(prepare_output["env"])
 elif config.android:
----------------
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.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D48309