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

[delcypher]

Summary

In order for this test to work the log file needs to be removed from both
from the host and device. To fix this the rm RUN lines have been
replaced with RUN: rm followed by RUN: %run rm.

Initially I tried having it so that RUN: %run rm implicitly runs rm
on the host as well so that only one RUN line is needed. This
simplified writing the test however that has two large drawbacks.

• It's potentially very confusing for use of the device scripts outside of the lit tests if asking for rm to run on device also causes files on the host to be deleted.

• This doesn't work well with the glob patterns used in the test. The host shell expands the %t.log.* glob pattern and not on the device so we could easily miss deleting old log files from previous test runs if the corresponding file doesn't exist on the host.

So instead deletion of files on the device and host are explicitly
separate commands.

To solve the globbing problem, the glob pattern passed to %run rm is
quoted so that the handling script can passed the unexpanded glob
pattern to the device so it can be expanded there. All implementations
of %run need to handle rm specially so that globbing patterns are
expanded on the device.

The iossim_run.py script has been fixed so that it just runs rm
on the host operating system because the device and host file system
are the same.

A visual inspection of android_run.py suggests that shell globbing
should work because it makes no attempt to quote paths when invoking
adb shell.

rdar://problem/41126835

[george.karpenkov]

test/sanitizer_common/ios_commands/iossim_run.py

33

Why not just use subprocess.call(.., shell=True) and let shell do the expansion?

[delcypher]

test/sanitizer_common/ios_commands/iossim_run.py

33

Seems reasonable. I'll make the change.

[delcypher]

Use shell=True mode of subprocess.call(...).

[filcab]

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.

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?), 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.

Thank you,
Filipe

test/ubsan/TestCases/Misc/log-path_test.cc

15

The device might not have a shell. And might be unable to run an rm command even if it has a shell.

[delcypher]

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

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.

test/ubsan/TestCases/Misc/log-path_test.cc

15

What device are you thinking of? Right now we only support Android and iOS-like devices which all have an accessible shell.

[george.karpenkov]

@filcab The script is in iossim folder, so it's specifically for iOS simulators. Arbitrary devices and their support is irrelevant for this change.

test/sanitizer_common/ios_commands/iossim_run.py

21

@dliew actually, stepping back a bit here, why can't we just do

subprocess.call(sys.argv[1:])

?

whatever shell expansions are there, they would be already expanded by the shell at that point.

[filcab]

In D51648#1224846, @delcypher wrote:

In 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

[george.karpenkov]

To run just-built programs for tests. Not to run anything else

What? Why?
How else can you run commands on the device (cat/cp/rm/etc) and why the semantics of those is different?
Many sanitizer tests already need those commands (e.g. this was recently done for libFuzzer to support device integration)

most will have a shell anyway

Yes, and having a shell seems to be a basic requirement for running LIT tests anyway.

I don't like forcing other platforms to do the same.

Where is this patch forcing other platforms?

[filcab]

In D51648#1225001, @george.karpenkov wrote:

@filcab The script is in iossim folder, so it's specifically for iOS simulators. Arbitrary devices and their support is irrelevant for this change.

Why do you say this?
It's changing test/ubsan/TestCases/Misc/log-path_test.cc assuming that %run rm will be useful for all targets.

[filcab]

In D51648#1225124, @george.karpenkov wrote:

To run just-built programs for tests. Not to run anything else

What? Why?
How else can you run commands on the device (cat/cp/rm/etc) and why the semantics of those is different?
Many sanitizer tests already need those commands (e.g. this was recently done for libFuzzer to support device integration)

Most lit commands are run on the host. The %run substitution is there for exactly that: "Take this just-built target executable and run it with these args".
All the cat/rm/etc commands are run on the host, as expected.

most will have a shell anyway

Yes, and having a shell seems to be a basic requirement for running LIT tests anyway.

lit tests run on the host, not the target.

I don't like forcing other platforms to do the same.

Where is this patch forcing other platforms?

The test is a general ubsan test.

[filcab]

In D51648#1224846, @delcypher wrote:

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 made me curious: Why don't the scripts delete the file after copying it to the host?
If the problem here is the extra files leftover, the scripts for iOS devices can handle it, as they already know which files need to be copied. Then we can even avoid adding anything here. It's a much more contained change.

Thank you,
Filipe

[delcypher]

In D51648#1225205, @filcab wrote:

In D51648#1224846, @delcypher wrote:

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 made me curious: Why don't the scripts delete the file after copying it to the host?

Several reasons

• Debug-ability. It's useful to examine the files left on the device when things go wrong.
• Convention. The scripts have never implicitly deleted files on a device (unless the command being run was a delete operation).

If the problem here is the extra files leftover, the scripts for iOS devices can handle it, as they already know which files need to be copied. Then we can even avoid adding anything here. It's a much more contained change.

Although that seems like a reasonable approach we are not going to do this. The scripts have never implicitly deleted files on the device and this is not behaviour we want to add. The test itself is the "source of truth" here. The test knows which files need to be deleted but
our device scripts in general do not. It is better to be explicit and have the test explicitly run a command to perform removal of files on the device.

I will change this patch to use the %device_rm substitution instead. For now I will make this operation be a no-op for everything except ios devices.

[delcypher]

Use %device_rm instead of %run rm.

[delcypher]

@filcab Is this new version of the patch better?

[vsk]

I think what %device_rm is doing is reasonably clear. I'd rather have this available than rely on the device correctly deleting files as needed.

I do wish that we could do something cleaner than substitute in 'echo' when %device_rm is unavailable, but don't have a better suggestion right now.

LGTM.

[filcab]

LGTM. Thank you!

Filipe

test/lit.common.cfg

112 ↗

(On Diff #165083)

Unsure if this is the best option, but it's ok for now. When I merge this to our tree I might post a patch to omit the warning on platforms that use "emulator" configs but don't need the device_rm.

[delcypher]

test/lit.common.cfg

112 ↗

(On Diff #165083)

Sounds good to me.

test/sanitizer_common/ios_commands/iossim_run.py

21

One possible reason not to do as you suggest is because file names might contain spaces which we couldn't pass to the shell verbatim without quoting. We also can't quote everything because we need to expand the shell globs. By design %device_rm takes shell glob patterns and is charge of expanding them for a device rather than relying on lit's underlying shell.

test/sanitizer_common/ios_commands/iossim_run.py

 #!/usr/bin/python
 
-import os, sys, subprocess
+import glob, os, pipes, sys, subprocess
 
 
 if not "SANITIZER_IOSSIM_TEST_DEVICE_IDENTIFIER" in os.environ:
   raise EnvironmentError("Specify SANITIZER_IOSSIM_TEST_DEVICE_IDENTIFIER to select which simulator to use.")
 
 device_id = os.environ["SANITIZER_IOSSIM_TEST_DEVICE_IDENTIFIER"]
 
 for e in ["ASAN_OPTIONS", "TSAN_OPTIONS", "UBSAN_OPTIONS"]:
   if e in os.environ:
     os.environ["SIMCTL_CHILD_" + e] = os.environ[e]
 
+prog = sys.argv[1]
+exit_code = None
+if prog == 'rm':
+  # The simulator and host actually share the same file system so we can just
+  # execute directly on the host.
+  rm_args = []
+  for arg in sys.argv[2:]:
+    if '*' in arg or '?' in arg:
+      # Don't quote glob pattern
+      rm_args.append(arg)
+    else:
+      # FIXME(dliew): pipes.quote() is deprecated
+      rm_args.append(pipes.quote(arg))
+  rm_cmd_line = ["/bin/rm"] + rm_args
+  rm_cmd_line_str = ' '.join(rm_cmd_line)
+  # We use `shell=True` so that any wildcard globs get expanded by the shell.
+  exitcode = subprocess.call(rm_cmd_line_str, shell=True)
+else:
   exitcode = subprocess.call(["xcrun", "simctl", "spawn", device_id] + sys.argv[1:])
 if exitcode > 125:
   exitcode = 126
 sys.exit(exitcode)

test/ubsan/TestCases/Misc/log-path_test.cc

 // FIXME: https://code.google.com/p/address-sanitizer/issues/detail?id=316
 // XFAIL: android
 
 // The globs below do not work in the lit shell.
 // REQUIRES: shell
 
 // RUN: %clangxx -fsanitize=undefined %s -O1 -o %t
 
 // Regular run.
 // RUN: %run %t -4 2> %t.out
 // RUN: FileCheck %s --check-prefix=CHECK-ERROR < %t.out
 
 // Good log_path.
 // RUN: rm -f %t.log.*
+// RUN: %run rm -f '%t.log.*'
 // RUN: %env_ubsan_opts=log_path='"%t.log"' %run %t -4 2> %t.out
 // RUN: FileCheck %s --check-prefix=CHECK-ERROR < %t.log.*
 
 // Run w/o errors should not produce any log.
 // RUN: rm -f %t.log.*
+// RUN: %run rm -f '%t.log.*'
 // RUN: %env_ubsan_opts=log_path='"%t.log"'  %run %t 4
 // RUN: not cat %t.log.*
 
 // FIXME: log_path is not supported on Windows yet.
 // XFAIL: windows-msvc
 
 #include <stdio.h>
 #include <stdlib.h>