[PATCH] D51648: [UBSan] Partially fix `test/ubsan/TestCases/Misc/log-path_test.cc` so that it can run on devices.

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added a comment.

In https://reviews.llvm.org/D51648#1224846, @delcypher wrote:

> In https://reviews.llvm.org/D51648#1224618, @filcab wrote:
>
> > I don't think this is acceptable.
> >  We have no guarantees we even have a shell on the devices. The run script might be doing all sort of things for commands to run on the device.
>
>
> That is true. However for all scripts that are in-tree where this test is meant to pass (currently only the iOS simulator) I've fixed them to work so that `%run rm` has a special meaning.
>  We have out-of-tree scripts for running on iOS devices and I've modified those to also handle the `%run rm` semantics that is being proposed in this patch.
>
> Out of tree consumers of compiler-rt that have their own device test infrastructure need (if this patch is merged) to specially handle `%run rm`.


`%run` has been used for one thing: To run just-built programs for tests. Not to run anything else. I don't think we should start to add more functionality to it, especially if we're just be going to special-case commands on it. We can just as easily create different substitutions. I see `%run` as having a very precise meaning, and not as a "this is a shell-like entry point to the device".

I'm ok with you overloading the iossim and ios scripts to do both and adding the `%device_rm` -> `%run rm` substitution. That's iossim and iOS saying "on our platform, we can do this". I don't like forcing other platforms (I'm actually there aren't that many platforms out of tree, and that most will have a shell anyway) to do the same.

It is also error prone when adding platforms, and people would need to go and dig out what other special commands they need to handle specially on `%run`.

>> Our approach has usually been to translate paths between host and device on our run script (or, in some tests, to add some code for our platform only). This is ok for us, but not everyone has the ability to have the host filesystem available.
>> 
>> If we *really* need something like this (what's going on when calling cat later? How do the files show up on the host?),
> 
> For the iOS simulator the host and the device file system are the same so no work is required. For iOS devices the scripts handle this by copying back the log file to the host when they detect `log_path=` was in ASAN_OPTIONS/TSAN_OPTIONS/UBSAN_OPTIONS.
>  This test is not meant to pass for Android so I've put no work in to make it work.
> 
>> maybe a %run_rm (or %device_rm, or whatever) would be better. Then platforms can do what they want and, if iossim_run and android_run can handle the rm command, they can add a substitution %device_rm -> %run rm.
> 
> I don't object to having a `%run_rm` substitution instead of `%run rm` but it's really not very different from requiring implementers of `%run` to support `rm` specially. Either way implementers have to implement something new so I don't really see the advantage of `%run_rm` really is.

Yes, it is different. Then the platform-specific substitutions are all together in the lit files. And `%run` is only called with absolute paths to programs to run on the target. Assuming fewer things from the target (when possible) is how we make sure we can support as many platforms as possible (or, at least, don't overburden embedded platforms with things like assuming a shell exists on the device).

Thank you,
Filipe


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D51648