This is an archive of the discontinued LLVM Phabricator instance.

Fix several test failures on Linux/FreeBSD caused by compiler configuration and invalid environment
ClosedPublic

Authored by ovyalov on Nov 24 2014, 1:05 PM.

Details

Reviewers
emaste
clayborg
Summary

Fix several test failures on Linux/FreeBSD caused by compiler configuration and invalid environment:

  1. TestSharedLib/TestSharedLibStrippedSymbols - set up explicit environment for target.LaunchSimple in case of Linux/FreeBSD.
  2. Fix compiler version detection - since CC environment variable may contain flags along with binary (e.g. "clang -Qunused-arguments -fcolor-diagnostics") we need to extract CC binary to allow which(..) to deduce correctly a compiler's full path .

Diff Detail

Event Timeline

ovyalov updated this revision to Diff 16574.Nov 24 2014, 1:05 PM
ovyalov retitled this revision from to Fix several test failures on Linux/FreeBSD caused by compiler configuration and invalid environment.
ovyalov updated this object.
ovyalov edited the test plan for this revision. (Show Details)
ovyalov added reviewers: clayborg, emaste.
ovyalov added a subscriber: Unknown Object (MLST).
emaste edited edge metadata.Nov 25 2014, 8:42 AM

Seems reasonable overall, with one comment.

test/lang/c/shared_lib/TestSharedLib.py
60–61

Will we need this on FreeBSD and Linux later on? I.e., do we need to append this to the environment?

Please see my comment.

Thank you!

test/lang/c/shared_lib/TestSharedLib.py
60–61

Presumably we may move Linux/BSD logic from here to lldbtest.py (https://github.com/llvm-mirror/lldb/blob/master/test/lldbtest.py#L1672) - as TODO suggests to expand this function with support for other platforms.
For now, we still need this environment variable to allow dynamic loader to find the shared library .

clayborg requested changes to this revision.Dec 1 2014, 7:15 AM
clayborg edited edge metadata.

Note the duplicated code in both the TestSharedLib.py and TestSharedLibStrippedSymbols.py test files. This work should all be done in the registerSharedLibrariesWithTarget() function. You can pass an array of full shared library paths to the self.registerSharedLibrariesWithTarget(...) function call, or the function knows how to locate the file in the build directory if you give it library base names.

This revision now requires changes to proceed.Dec 1 2014, 7:15 AM
ovyalov updated this revision to Diff 16784.Dec 1 2014, 1:09 PM
ovyalov edited edge metadata.

Thanks for suggestion - reworked tests to make registerSharedLibrariesWithTarget to support multiple platforms.
Please take another look.

clayborg requested changes to this revision.Dec 1 2014, 1:18 PM
clayborg edited edge metadata.

The remote install file spec only needs to be set for remote targets and you must use the "lldb.remote_platform.GetWorkingDirectory()" as the directory (since it represents the remote install location) instead of "working_dir" which is on the current host.

To fix this you need to change:

  1. We must set the remote install location if we want the shared library
  2. to get uploaded to the remote target

remote_shlib_path = os.path.join(working_dir, os.path.basename(local_shlib_path))
shlib_module.SetRemoteInstallFileSpec(lldb.SBFileSpec(remote_shlib_path, False))

to be:

if lldb.remote_platform:

  1. We must set the remote install location if we want the shared library
  2. to get uploaded to the remote target remote_shlib_path = os.path.join(lldb.remote_platform.GetWorkingDirectory(), os.path.basename(local_shlib_path)) shlib_module.SetRemoteInstallFileSpec(lldb.SBFileSpec(remote_shlib_path, False))
This revision now requires changes to proceed.Dec 1 2014, 1:18 PM

Try this again with correct formatting:

The remote install file spec only needs to be set for remote targets and you must use the "lldb.remote_platform.GetWorkingDirectory()" as the directory (since it represents the remote install location) instead of "working_dir" which is on the current host.

To fix this you need to change:

# We must set the remote install location if we want the shared library
remote_shlib_path = os.path.join(working_dir, os.path.basename(local_shlib_path))
shlib_module.SetRemoteInstallFileSpec(lldb.SBFileSpec(remote_shlib_path, False))

To:

if lldb.remote_platform:
    # We must set the remote install location if we want the shared library
    # to get uploaded to the remote target
    remote_shlib_path = os.path.join(lldb.remote_platform.GetWorkingDirectory(), os.path.basename(local_shlib_path))
    shlib_module.SetRemoteInstallFileSpec(lldb.SBFileSpec(remote_shlib_path, False))
ovyalov updated this revision to Diff 16787.Dec 1 2014, 1:58 PM
ovyalov edited edge metadata.

Thanks for catching this - fixed.

How I can run tests with remote_platform tuned on? My understanding that lldb.remote_platform is initialized from platform-name/platform-url/platform-working-dir dotest flags, but if you have an example of command line for remote platform - could you share it please?

clayborg accepted this revision.Dec 1 2014, 2:43 PM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Dec 1 2014, 2:43 PM
zturner added inline comments.
test/lldbtest.py
747–750

This breaks windows because a platformContext is not set for Windows. Do you have suggestions on how to fix this for Windows? Perhaps a default implementation that does nothing?

I posted a proposed fix for this, please take a look.

http://reviews.llvm.org/D6484

Check my comments which you will want to fix with a new fix.

What's the next step with this change?

ovyalov closed this revision.Aug 30 2015, 11:05 PM

Closed with r223091