[lldbtests] Handle errors instead of crashing

[davide]

Summary

If you pass an invalid compiler/debugger path on the cmdline to dotest.py this is what you get.

$ python dotest.py --executable /home/davide/work/build-lldb/bin/lldb --compiler /home/davide/work/build-lldb/bin/clandasfaasdfsg
Traceback (most recent call last):
  File "dotest.py", line 7, in <module>
    lldbsuite.test.run_suite()
  File "/home/davide/work/llvm-lldb/tools/lldb/packages/Python/lldbsuite/test/dotest.py", line 1099, in run_suite
    parseOptionsAndInitTestdirs()
  File "/home/davide/work/llvm-lldb/tools/lldb/packages/Python/lldbsuite/test/dotest.py", line 282, in parseOptionsAndInitTestdirs
    if not is_exe(configuration.compiler):
  File "/home/davide/work/llvm-lldb/tools/lldb/packages/Python/lldbsuite/test/dotest.py", line 54, in is_exe
    return os.path.isfile(fpath) and os.access(fpath, os.X_OK)
  File "/usr/lib64/python2.7/genericpath.py", line 37, in isfile
    st = os.stat(path)
TypeError: coercing to Unicode: need string or buffer, NoneType found

And with the patch applied:

$ python dotest.py --executable /home/davide/work/build-lldb/bin/lldb --compiler /home/davide/work/build-lldb/bin/clandasfasg
/home/davide/work/build-lldb/bin/clandasfasg is not a valid path, exiting

Please let me know what you think.

[lemo]

packages/Python/lldbsuite/test/dotest.py

56 ↗

(On Diff #119905)

minor: can we print quotes around fpath? (this usually helps in messages to avoid confusion around paths containing spaces for ex)

[davide]

packages/Python/lldbsuite/test/dotest.py

56 ↗

(On Diff #119905)

Sure, I'll change that before committing if people like the approach.

[zturner]

Can you upload diffs with context in the future?

I'm trying to figure out whether is_exe is used for anything where non-existance should not be considered fatal, but I can't see the rest of the file here.

[davide]

Apologies, I generally do (but I forgot this time), let me update the patch.

[davide]

If there are cases where is_exe shouldn't be fatal, then we might consider splitting the utility into two bits (exists & is_exe).
In my mind, if you're checking whether something is executable you should run the check on a valid entity, and it's up to the caller guaranteeing the validity (we should, of course, check here).

[clayborg]

packages/Python/lldbsuite/test/dotest.py

52 ↗

(On Diff #119917)

We could add a default parameter here like:

def is_exe(fpath, fatal=False):
  if fatal and not os.path.exists(fpath):
     ...

Then modify any callers. For the --compiler option we should verify it does exist and it should be fatal. Not sure how many other places use this.

[davide]

packages/Python/lldbsuite/test/dotest.py

52 ↗

(On Diff #119917)

Is there any use case where we want to have this relaxed behaviour?
FWIW, I see this being checked in two cases:

1. cmdline handling, in which case can have typing mistakes. In that case, I think we should just exit.
2. input from other scripts. In that case, I think the test scripts implicitly guarantee the validity of the path/handle [and if the path is invalid, it's much nicer to just print a diagnostic instead of an unhandled exception trace, IMHO].

In other words, I'm not entirely convinced we want an additional argument unless there's a good reason for it.

[zturner]

I just wanted to make sure nobody was *already* relying on that behavior. If we can get away with being strict, we should be strict.

[davide]

In D39199#904169, @zturner wrote:

I just wanted to make sure nobody was *already* relying on that behavior. If we can get away with being strict, we should be strict.

I agree. From a quick skim I don't see anything really relying on that, but another look would be really appreciated (in particular if it's from somebody more familiar with this codebase than I am)

[jasonmolenda]

I think this is a good idea, thanks for writing the patch Davide.

[labath]

Davide, I reverted this because it breaks the case when you just specify a filename as a compiler (with the expectation that we will look it up in the path). I think this is a good thing to have, and our buildbot was using that behavior.

I like the direction this change was going in, but I couldn't find a simple way to fix this use case, as is_exe is being used from a number of places, so I reverted the change for now.

lldb/trunk/packages/Python/lldbsuite/test/dotest.py

 from . import test_categories
 from lldbsuite.test_event import formatter
 from . import test_result
 from lldbsuite.test_event.event_builder import EventBuilder
 from ..support import seven
 
 
 def is_exe(fpath):
-    """Returns true if fpath is an executable."""
+    """Returns true if fpath is an executable.
+       Exits with an error code if the specified path is invalid"""
+    if not os.path.exists(fpath):
+        print(fpath  + " is not a valid path, exiting")
+        sys.exit(-1)
     return os.path.isfile(fpath) and os.access(fpath, os.X_OK)
 
 
 def which(program):
     """Returns the full path to a program; None otherwise."""
     fpath, fname = os.path.split(program)
     if fpath:
         if is_exe(program):