Page MenuHomePhabricator

[bugpoint] Find 'opt', etc., in bugpoint directory

Authored by modocache on Nov 25 2018, 8:51 PM.



When bugpoint attempts to find the other executables it needs to run,
such as opt or clang, it tries searching the user's PATH. However,
in many cases, the 'bugpoint' executable is part of an LLVM build, and
the 'opt' executable it's looking for is in that same directory.

Many LLVM tools handle this case by using the Paths parameter of
llvm::sys::findProgramByName, passing the parent path of the currently
running executable. Do this same thing for bugpoint. However, to
preserve the current behavior exactly, first search the user's PATH,
and then search for 'opt' in the directory containing 'bugpoint'.

Test Plan:
check-llvm. Many of the existing bugpoint tests no longer need to use the
--opt-command option as a result of these changes.

Diff Detail


Event Timeline

modocache created this revision.Nov 25 2018, 8:51 PM

Friendly ping! I like this patch because it saves me the keystrokes of typing --opt-command every time I run bugpoint. Please give it a look and let me know if I can improve anything.

@MatzeB, I noticed you had created a few bugpoint bugs on Bugzilla related to bugpoint; would you be a good person to review this?

davide added a subscriber: davide.Nov 28 2018, 8:30 AM

As somebody who used bugpoint fairly extensively over the course of the years, I have to say I always considered a little error prone the fact that bugpoint looks up in the PATH to find opt.
So, I tend to be in favor of this patch, but I'm wondering whether it's worth to look in the $PATH instead of just looking in the current directory.
Realistically bugpoint is a tool for developers, so chances are you're going to run it directly from bin using the compiler/interpreter/assembler you just built.

You might consider asking on llvm-dev how people use the tool, and if there are no strong objections, we might be able to change the behavior. Other than that, the patch looks straightforward enough.

davide accepted this revision.Dec 1 2018, 12:42 PM

LGTM conditional to lack of objections from people on llvm-dev.

This revision is now accepted and ready to land.Dec 1 2018, 12:42 PM
MatzeB accepted this revision.Dec 2 2018, 5:41 PM

I certainly was always annoyed that bugpoint wouldn't pick up the tools in the same build directory forcing me to manually specify paths to the tools.
However unfortunately this isn't going far enough and will still search the tools in PATH first, which can still be annoying as in my experience you can often have llvm developer tools installed by your distribution/package managers etc. in your PATH.

So while I'm fine applying this patch (with the test problems addressed), I'd even vote to search inside the bugpoint directory first before checking the PATH. Happy to hear some other opinions though and/or apply this in the meantime.

1 ↗(On Diff #175195)

Won't this pick up an opt in the PATH now if one is available? That would make the test unstable.

Similar in the other tests.

modocache marked an inline comment as done.Dec 9 2018, 8:44 AM

Thanks @MatzeB! I'll change this to search in the bugpoint directory first, and fall back to the PATH.

I also responded to your comments on the test. As I mention in the comment there, I think that once I change the search behavior and confirm all tests still pass, this patch seems good enough to commit. Let me know if you disagree!

1 ↗(On Diff #175195)

Oh, I see. I didn't realize that LLVM's test/ provides a substitution named opt -- so this actually expands to the lit command --opt-command /path/to/build/bin/opt:

# llvm/test/

    # ...
    'verify-uselistorder', 'bugpoint', 'llc', 'llvm-symbolizer', 'opt',
    'sancov', 'sanstats'])

# ...

llvm_config.add_tool_substitutions(tools, config.llvm_tools_dir)

However, I believe the tests are still stable after this change, and here's why: test/ also inserts the built LLVM tools directory at the beginning of the PATH when running tests:

# Tweak the PATH to include the tools dir.
llvm_config.with_environment('PATH', config.llvm_tools_dir, append_path=True)

So during the test run, /path/to/build/bin is always the first item in the PATH. Since the default behavior of bugpoint is to try and find opt in the PATH when no --opt-command is specified, removing this option has no material impact on the tests.

So this means two things:

  1. The usages of --opt-command in the bugpoint tests could all be removed today, even without this patch, as long as continues to prepend the tools directory to the PATH when running tests.
  2. Since these options can be removed even without this patch, removing them isn't a sufficient way to test the behavior in this diff. Still, I think removing the usage of the options, and changing the behavior of bugpoint so it searches in the bugpoint executable's directory first, might be a "good enough" test of the new behavior. The only alternative I can think of is having a test that copies the bugpoint executable into a temporary directory, copying a test programmed named "opt" into that same directory, and invoking bugpoint in some way that demonstrates the "opt" from the temporary directory is chosen... and that seems like overkill to test this minor convenience.
modocache updated this revision to Diff 177436.Dec 9 2018, 8:48 AM

Search the bugpoint directory first, and only fall back to PATH if the program isn't there.

modocache marked an inline comment as done.Dec 9 2018, 9:04 AM
This revision was automatically updated to reflect the committed changes.