Skip to content

[lldb] Fix write only file action to truncate the file #112657

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lldb/source/Host/common/FileAction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ bool FileAction::Open(int fd, const FileSpec &file_spec, bool read,
else if (read)
m_arg = O_NOCTTY | O_RDONLY;
else
m_arg = O_NOCTTY | O_CREAT | O_WRONLY;
m_arg = O_NOCTTY | O_CREAT | O_WRONLY | O_TRUNC;
m_file_spec = file_spec;
return true;
} else {
Expand Down
53 changes: 53 additions & 0 deletions lldb/test/API/commands/settings/TestSettings.py
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,59 @@ def test_set_error_output_path(self):
output, exe=False, startstr="This message should go to standard out."
)

@skipIfDarwinEmbedded # <rdar://problem/34446098> debugserver on ios etc can't write files
def test_same_error_output_path(self):
"""Test that setting target.error and output-path to the same file path for the launched process works."""
self.build()

exe = self.getBuildArtifact("a.out")
self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)

# Set the error-path and output-path and verify both are set.
self.runCmd(
"settings set target.error-path '{0}'".format(
lldbutil.append_to_process_working_directory(self, "output.txt")
)
)
self.runCmd(
"settings set target.output-path '{0}".format(
lldbutil.append_to_process_working_directory(self, "output.txt")
)
)
# And add hooks to restore the original settings during tearDown().
self.addTearDownHook(lambda: self.runCmd("settings clear target.output-path"))
self.addTearDownHook(lambda: self.runCmd("settings clear target.error-path"))

self.expect(
"settings show target.error-path",
SETTING_MSG("target.error-path"),
substrs=["target.error-path (file)", 'output.txt"'],
)

self.expect(
"settings show target.output-path",
SETTING_MSG("target.output-path"),
substrs=["target.output-path (file)", 'output.txt"'],
)

self.runCmd(
"process launch --working-dir '{0}'".format(
self.get_process_working_directory()
),
RUN_SUCCEEDED,
)

output = lldbutil.read_file_from_process_wd(self, "output.txt")
err_message = "This message should go to standard error."
out_message = "This message should go to standard out."
# Error msg should get flushed by the output msg
self.expect(output, exe=False, substrs=[out_message])
self.assertNotIn(
err_message,
output,
"Race condition when both stderr/stdout redirects to the same file",
)

def test_print_dictionary_setting(self):
self.runCmd("settings clear target.env-vars")
self.runCmd('settings set target.env-vars ["MY_VAR"]=some-value')
Expand Down
30 changes: 30 additions & 0 deletions lldb/test/API/python_api/process/io/TestProcessIO.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,36 @@ def test_stdout_stderr_redirection(self):
error = self.read_error_file_and_delete()
self.check_process_output(output, error)

@skipIfWindows # stdio manipulation unsupported on Windows
@expectedFlakeyLinux(bugnumber="llvm.org/pr26437")
@skipIfDarwinEmbedded # debugserver can't create/write files on the device
def test_stdout_stderr_redirection_to_existing_files(self):
"""Exercise SBLaunchInfo::AddOpenFileAction() for STDOUT and STDERR without redirecting STDIN to output files already exist."""
self.setup_test()
self.build()
self.create_target()
self.write_file_with_placeholder(self.output_file)
self.write_file_with_placeholder(self.error_file)
self.redirect_stdout()
self.redirect_stderr()
self.run_process(True)
output = self.read_output_file_and_delete()
error = self.read_error_file_and_delete()
self.check_process_output(output, error)

def write_file_with_placeholder(self, target_file):
placeholder = "This content should be overwritten."
if lldb.remote_platform:
self.runCmd(
'platform file write "{target}" -d "{data}"'.format(
target=target_file, data=placeholder
)
)
else:
f = open(target_file, "w")
f.write(placeholder)
f.close()

# target_file - path on local file system or remote file system if running remote
# local_file - path on local system
def read_file_and_delete(self, target_file, local_file):
Expand Down
25 changes: 25 additions & 0 deletions lldb/unittests/Host/FileActionTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
//
//===----------------------------------------------------------------------===//

#include <fcntl.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work on windows?

Copy link
Contributor Author

@kusmour kusmour Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, from the code search it seems a lot of base classes have that #include. I don't have a windows machine to test with. I can change the #include to a file within llvm-project that has the flag definition
eg. https://github.com/llvm/llvm-project/blob/main/lldb/source/Host/common/File.cpp#L15-L19

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also passing the Windows CI. @bulbazord I can help @kusmour validate if you want, but I personally think CI is good enough

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is good enough for me here. I've approved the PR.


#include "lldb/Host/FileAction.h"
#include "gtest/gtest.h"

Expand All @@ -17,3 +19,26 @@ TEST(FileActionTest, Open) {
EXPECT_EQ(Action.GetAction(), FileAction::eFileActionOpen);
EXPECT_EQ(Action.GetFileSpec(), FileSpec("/tmp"));
}

TEST(FileActionTest, OpenReadWrite) {
FileAction Action;
Action.Open(48, FileSpec("/tmp_0"), /*read*/ true, /*write*/ true);
EXPECT_TRUE(Action.GetActionArgument() & (O_NOCTTY | O_CREAT | O_RDWR));
EXPECT_FALSE(Action.GetActionArgument() & O_RDONLY);
EXPECT_FALSE(Action.GetActionArgument() & O_WRONLY);
}

TEST(FileActionTest, OpenReadOnly) {
FileAction Action;
Action.Open(49, FileSpec("/tmp_1"), /*read*/ true, /*write*/ false);
EXPECT_TRUE(Action.GetActionArgument() & (O_NOCTTY | O_RDONLY));
EXPECT_FALSE(Action.GetActionArgument() & O_WRONLY);
}

TEST(FileActionTest, OpenWriteOnly) {
FileAction Action;
Action.Open(50, FileSpec("/tmp_2"), /*read*/ false, /*write*/ true);
EXPECT_TRUE(Action.GetActionArgument() &
(O_NOCTTY | O_CREAT | O_WRONLY | O_TRUNC));
EXPECT_FALSE(Action.GetActionArgument() & O_RDONLY);
}
2 changes: 2 additions & 0 deletions llvm/docs/ReleaseNotes.md
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,8 @@ Changes to LLDB
* LLDB can now read the `fpmr` register from AArch64 Linux processes and core
files.

* Program stdout/stderr redirection will now open the file with O_TRUNC flag, make sure to truncate the file if path already exists.
* eg. `settings set target.output-path/target.error-path <path/to/file>`

Changes to BOLT
---------------------------------
Expand Down
Loading