[LLDB] Add a target.launch-working-dir setting #113521
Conversation
|
@llvm/pr-subscribers-lldb Author: Walter Erquinigo (walter-erquinigo) ChangesInternally we use bazel in a way in which it can drop you in a LLDB session with the target launched in a particular cwd, which is needed for things to work. We've been making this automation work via Full diff: https://github.com/llvm/llvm-project/pull/113521.diff 5 Files Affected:
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index e4848f19e64d62..0e8bbb41f29941 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -37,6 +37,7 @@
#include "lldb/Utility/RealpathPrefixes.h"
#include "lldb/Utility/Timeout.h"
#include "lldb/lldb-public.h"
+#include "llvm/ADT/StringRef.h"
namespace lldb_private {
@@ -114,6 +115,8 @@ class TargetProperties : public Properties {
void SetDisableSTDIO(bool b);
+ std::optional<llvm::StringRef> GetLaunchWorkingDirectory() const;
+
const char *GetDisassemblyFlavor() const;
InlineStrategy GetInlineStrategy() const;
diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp
index e7c7d07ad47722..297c94c1f0a055 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -201,6 +201,11 @@ class CommandObjectProcessLaunch : public CommandObjectProcessLaunchOrAttach {
if (target->GetDisableSTDIO())
m_options.launch_info.GetFlags().Set(eLaunchFlagDisableSTDIO);
+ if (std::optional<llvm::StringRef> wd =
+ target->GetLaunchWorkingDirectory()) {
+ m_options.launch_info.SetWorkingDirectory(FileSpec(*wd));
+ }
+
// Merge the launch info environment with the target environment.
Environment target_env = target->GetEnvironment();
m_options.launch_info.GetEnvironment().insert(target_env.begin(),
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 04395e37f0425d..ef4dabf00c1a9e 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -4428,6 +4428,15 @@ void TargetProperties::SetDisableSTDIO(bool b) {
const uint32_t idx = ePropertyDisableSTDIO;
SetPropertyAtIndex(idx, b);
}
+std::optional<llvm::StringRef>
+TargetProperties::GetLaunchWorkingDirectory() const {
+ const uint32_t idx = ePropertyLaunchWorkingDir;
+ llvm::StringRef value = GetPropertyAtIndexAs<llvm::StringRef>(
+ idx, g_target_properties[idx].default_cstr_value);
+ if (value.empty())
+ return {};
+ return value;
+}
const char *TargetProperties::GetDisassemblyFlavor() const {
const uint32_t idx = ePropertyDisassemblyFlavor;
diff --git a/lldb/source/Target/TargetProperties.td b/lldb/source/Target/TargetProperties.td
index fb61478fb752dc..613442501d6b6d 100644
--- a/lldb/source/Target/TargetProperties.td
+++ b/lldb/source/Target/TargetProperties.td
@@ -201,6 +201,10 @@ let Definition = "target" in {
def DebugUtilityExpression: Property<"debug-utility-expression", "Boolean">,
DefaultFalse,
Desc<"Enable debugging of LLDB-internal utility expressions.">;
+ def LaunchWorkingDir: Property<"launch-working-dir", "String">,
+ DefaultStringValue<"">,
+ Desc<"An override for the working directory to use when launching processes. "
+ "It's not used when empty.">;
}
let Definition = "process_experimental" in {
diff --git a/lldb/test/API/commands/process/launch/TestProcessLaunch.py b/lldb/test/API/commands/process/launch/TestProcessLaunch.py
index 45f9f494ab8f5c..f3afc385a56086 100644
--- a/lldb/test/API/commands/process/launch/TestProcessLaunch.py
+++ b/lldb/test/API/commands/process/launch/TestProcessLaunch.py
@@ -8,6 +8,7 @@
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
from lldbsuite.test import lldbutil
+from pathlib import Path
class ProcessLaunchTestCase(TestBase):
@@ -206,3 +207,47 @@ def test_environment_with_special_char(self):
self.assertEqual(value, evil_var)
process.Continue()
self.assertState(process.GetState(), lldb.eStateExited, PROCESS_EXITED)
+
+ def test_target_launch_working_dir_prop(self):
+ """Test that the setting `target.launch-working-dir` is correctly used when launching a process."""
+ d = {"CXX_SOURCES": "print_cwd.cpp"}
+ self.build(dictionary=d)
+ self.setTearDownCleanup(d)
+ exe = self.getBuildArtifact("a.out")
+ self.runCmd("file " + exe)
+
+ mywd = "my_working_dir"
+ out_file_name = "my_working_dir_test.out"
+
+ my_working_dir_path = self.getBuildArtifact(mywd)
+ lldbutil.mkdir_p(my_working_dir_path)
+ out_file_path = os.path.join(my_working_dir_path, out_file_name)
+ another_working_dir_path = Path(
+ os.path.join(my_working_dir_path, "..")
+ ).resolve()
+
+ # Check that we correctly set the working dir
+ self.runCmd(
+ f"settings set target.launch-working-dir {another_working_dir_path}"
+ )
+ launch_command = f"process launch -o {out_file_path}"
+
+ self.expect(
+ launch_command,
+ patterns=["Process .* launched: .*a.out"],
+ )
+
+ out = lldbutil.read_file_on_target(self, out_file_path)
+ self.assertIn(f"stdout: {another_working_dir_path}", out)
+
+ # Check that we can unset the setting
+ self.runCmd(f"settings set target.launch-working-dir ''")
+ launch_command = f"process launch -o {out_file_path}"
+
+ self.expect(
+ launch_command,
+ patterns=["Process .* launched: .*a.out"],
+ )
+
+ out = lldbutil.read_file_on_target(self, out_file_path)
+ self.assertNotIn(f"stdout: {another_working_dir_path}", out)
|
walter-erquinigo
force-pushed
the
users/walter-erquinigo/run
branch
from
be5c3c3 to
4c80e26
Compare
|
Friendly ping, otherwise I'll merge it in a couple of days |
|
This issue could be re-stated as "a process launched with -w does not use the same working dir if run again". Which I think is certainly unexpected behaviour as a user. Did you look into making The way I see it this splits in two:
Feels like you are solving one problem using the solution of a different problem. |
That's a good idea
Another good idea
That was just a symptom, but what we want is users not having to specify
Sadly it seems that this is not possible with the way bazel spawns tools, which respects the cwd from the user invocation. We can invoke LLDB with any init flags we want, but we can't change its actual cwd. That's how I thought of adding the working-dir as a setting via the command line invocation. What do you think? |
We have:
You will have to decide whether
Although one might think that So let's leave the
On second thought, if this is on target then the launch workdir dir should be too: ...so ignore advice from me on a Wednesday I guess :) Also you could reference how this setting interacts with the other ways to pass arguments like |
Even if this way of using it (I have no experience with Bazel myself) is unusual, I think the pitch for the setting is ok anyway. |
lldb/source/Target/Target.cpp
Outdated
| @@ -4428,6 +4428,15 @@ void TargetProperties::SetDisableSTDIO(bool b) { | |||
| const uint32_t idx = ePropertyDisableSTDIO; | |||
| SetPropertyAtIndex(idx, b); | |||
| } | |||
| std::optional<llvm::StringRef> | |||
| TargetProperties::GetLaunchWorkingDirectory() const { | |||
There was a problem hiding this comment.
If it's not used when empty should this just return llvm::StringRef?
I get the logical purity of using optional, but if we're defining some value of it as the "null" value anyway we don't have to use optional.
There was a problem hiding this comment.
Yeah, that makes sense, also because of consistency with the rest of the code
| def LaunchWorkingDir: Property<"launch-working-dir", "String">, | ||
| DefaultStringValue<"">, | ||
| Desc<"A default value for the working directory to use when launching processes. " | ||
| "It's not used when empty.">; |
There was a problem hiding this comment.
Here we should say how it interacts with commands that can set the working dir, and that changing this at runtime does not change the running process' working dir.
"This setting is only used when the target is launched. If you change this setting, the new value will only apply to subsequent launches."
| os.path.join(my_working_dir_path, "..") | ||
| ).resolve() | ||
|
|
||
| # Check that we correctly override the working dir |
There was a problem hiding this comment.
Please rewrite this so it's obvious what "correct" means. Like:
# If -w is passed to process launch, that value will be used instead of the setting.
| out = lldbutil.read_file_on_target(self, out_file_path) | ||
| self.assertNotIn(f"stdout: {another_working_dir_path}", out) | ||
|
|
||
| # Check that we correctly set the working dir |
There was a problem hiding this comment.
Same here, state what you expect to happen.
| out = lldbutil.read_file_on_target(self, out_file_path) | ||
| self.assertIn(f"stdout: {another_working_dir_path}", out) | ||
|
|
||
| # Check that we can unset the setting |
|
Also please add a release note to the LLDB section in the llvm release notes doc. These sort of settings people don't know they could use, until they realise they exist. |
Actually this is not a bug. I think if you should add to this help text: To note that it only applies to that launch and that anyone who wants a sticky setting should use this new setting. |
One week is the minimum time for a ping (you pinged after six days). It's not the maximum time for a review (there isn't one). (Practically speaking, if you feel your reviews are going slower than usual, it could be because a lot of the devs were at the conference last week. I'm still catching up with the backlog.) |
|
Sorry, I thought it had been 1 week already. I'll be mindful of that next time. And yeah, it's natural that the conference slowed things down. I just thought that people weren't caring about this patch, which happens very occasionally :) |
walter-erquinigo
force-pushed
the
users/walter-erquinigo/run
branch
from
4c80e26 to
819380f
Compare
There was a problem hiding this comment.
We only have one request for this feature, but the logic of it is clear and GDB as an equivalent in set cwd: https://sourceware.org/gdb/current/onlinedocs/gdb.html/Working-Directory.html
So this looks good to me once the docs are updated.
Internally we use bazel in a way in which it can drop you in a LLDB session with the target launched in a particular cwd, which is needed for things to work. We've been making this automation work via `process launch -w`. However, if later the user wants to restart the process with `r`, then they end up using a different cwd for relaunching the process. As a way to fix this, I'm adding a target-level setting that allows setting a default cwd used for launching the process without needing the user to specify it manually.
walter-erquinigo
force-pushed
the
users/walter-erquinigo/run
branch
from
819380f to
5c78bec
Compare
|
@DavidSpickett , thanks for the thorough review! It was great to see that gdb has a similar functionality as well. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/195/builds/622 Here is the relevant piece of the build log for the reference |
This reverts commit 6620cd2.
Reverts #113521 due to build bot failures mentioned in the original PR.
Internally we use bazel in a way in which it can drop you in a LLDB session with the target launched in a particular cwd, which is needed for things to work. We've been making this automation work via `process launch -w`. However, if later the user wants to restart the process with `r`, then they end up using a different cwd for relaunching the process. As a way to fix this, I'm adding a target-level setting that allows configuring a default cwd used for launching the process without needing the user to specify it manually.
Reverts llvm#113521 due to build bot failures mentioned in the original PR.
Internally we use bazel in a way in which it can drop you in a LLDB session with the target launched in a particular cwd, which is needed for things to work. We've been making this automation work via
process launch -w. However, if later the user wants to restart the process withr, then they end up using a different cwd for relaunching the process. As a way to fix this, I'm adding a target-level setting that allows configuring a default cwd used for launching the process without needing the user to specify it manually.