This is an archive of the discontinued LLVM Phabricator instance.

[lldb][test] Prevent infinite loop while looking for use_lldb_suite_root.py.
ClosedPublic

Authored by rupprecht on Jul 14 2020, 6:45 PM.

Details

Summary

use_lldb_suite.py looks for use_lldb_suite_root.py by checking parent directories. If for some reason it doesn't exist, it keeps checking parent directories until it finds it.

However, this only breaks when the parent directory is None, but at least on Linux, dirname('/') == '/', so this will never be None.

This changes the lookup to stop if the dirname(lldb_root) is unchanged. It still stops if dirname returns None on some systems.

Additionally, this makes the failure mode more visible -- if the root is not found, it complains loudly instead of silently failing, having later modules that need lldb_root fail.

Diff Detail

Event Timeline

rupprecht created this revision.Jul 14 2020, 6:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2020, 6:45 PM
labath accepted this revision.Jul 15 2020, 2:06 AM

Heh I fixed the same thing back in 67f6d842fab6 for scripts/use_lldb_suite.py, but of course I forgot to update this copy... The idea is good, just please make sure both files end up using the same solution.

(Also, if you have any ideas on how to avoid having multiple copies of this file around, then don't keep them to yourself. :) )

This revision is now accepted and ready to land.Jul 15 2020, 2:06 AM

Heh I fixed the same thing back in 67f6d842fab6 for scripts/use_lldb_suite.py, but of course I forgot to update this copy... The idea is good, just please make sure both files end up using the same solution.

Oh, and there's another one -- lldb/packages/Python/lldbsuite/__init__.py. I'll update it too.

(Also, if you have any ideas on how to avoid having multiple copies of this file around, then don't keep them to yourself. :) )

Not at the moment, but I have a feeling I'll be staring at these files long enough to come up with something.

This revision was automatically updated to reflect the committed changes.
shafik added a subscriber: shafik.Jul 15 2020, 1:29 PM

@labath why do we need two copies of use_lldb_suite.py?

@labath why do we need two copies of use_lldb_suite.py?

This script is responsible for setting up an appropriate python import path. Before we can import any code in lldb/third_party/Python/module or lldb/packages/Python, we need to add those paths to sys.path. And we cannot completely put this code into a central place because then we wouldn't know how to import that.

So, the way get around that is by placing this file into the same folder as the script that needs it. Then, the script can load this file using a relative import, and afterwards, it can import anything it wants. And since we have scripts needing this functionality in multiple places, we have multiple copies of the script.

At least that's the current state of the art. It's possible that there are better solutions, but we just don't know about them.

I wouldn't say this is a major problem. These files don't get a lot of traffic -- this is pretty much the second change since they were added five years ago, and the only reason we ran into this is because it caused problems in our weird internal distributed build setup. But removing the duplication would certainly be nice...

@labath why do we need two copies of use_lldb_suite.py?

This script is responsible for setting up an appropriate python import path. Before we can import any code in lldb/third_party/Python/module or lldb/packages/Python, we need to add those paths to sys.path. And we cannot completely put this code into a central place because then we wouldn't know how to import that.

So, the way get around that is by placing this file into the same folder as the script that needs it. Then, the script can load this file using a relative import, and afterwards, it can import anything it wants. And since we have scripts needing this functionality in multiple places, we have multiple copies of the script.

At least that's the current state of the art. It's possible that there are better solutions, but we just don't know about them.

We could have CMake configure this at the cost of always having to use the corresponding scripts from the build directory? It seems like only analyze-project-deps.py and host_art_bt.py are importing lldb right now so it might be worth considering.

I wouldn't say this is a major problem. These files don't get a lot of traffic -- this is pretty much the second change since they were added five years ago, and the only reason we ran into this is because it caused problems in our weird internal distributed build setup. But removing the duplication would certainly be nice...

@labath why do we need two copies of use_lldb_suite.py?

This script is responsible for setting up an appropriate python import path. Before we can import any code in lldb/third_party/Python/module or lldb/packages/Python, we need to add those paths to sys.path. And we cannot completely put this code into a central place because then we wouldn't know how to import that.

So, the way get around that is by placing this file into the same folder as the script that needs it. Then, the script can load this file using a relative import, and afterwards, it can import anything it wants. And since we have scripts needing this functionality in multiple places, we have multiple copies of the script.

At least that's the current state of the art. It's possible that there are better solutions, but we just don't know about them.

We could have CMake configure this at the cost of always having to use the corresponding scripts from the build directory? It seems like only analyze-project-deps.py and host_art_bt.py are importing lldb right now so it might be worth considering.

The important part is who is importing use_lldb_suite, not lldb. By the looks of things host_art_bt.py is meant to be included from within lldb, so the path will be correct there.

Not being able to run analyze-project-deps from the source tree would be a minor inconvenience, so I'm not sure if it's worth it. But I am not opposed to the idea either. All that script really needs is the location of the repo root. Since it already has a lot of knowledge about the repo layout, we could just hardcode that too...

I don't know if it's the only script that needs that...