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

kusmour · 2024-10-17T03:35:33Z

When FileAction opens file with write access, it doesn't clear the file nor append to the end of the file if it already exists. Instead, it writes from cursor index 0.

For example, by using the settings target.output-path and target.error-path, lldb will redirect process stdout/stderr to files. It then calls this function to write to the files which the above symptoms appear.

Test

Added unit test checking the file flags
Added 2 api tests checking
- File content overwritten if the file path already exists
- Stdout and stderr redirection to the same file doesn't change its behavior

llvmbot · 2024-10-17T03:36:06Z

@llvm/pr-subscribers-lldb

Author: Wanyi (kusmour)

Changes

When FileAction opens file with write access for lldb stdout/stderr redirect, it doesn't clear the file nor append to the end of the file if it already exists. Instead, it writes from cursor index 0.

This function is only called when launch debugging with stdin/stdout/stderr redirection to files.

We can also implement open options like the FileSystem::Open method so that we can handle truncate vs. append

Full diff: https://github.com/llvm/llvm-project/pull/112657.diff

2 Files Affected:

(modified) lldb/source/Host/common/FileAction.cpp (+1-1)
(modified) lldb/unittests/Host/FileActionTest.cpp (+24)

diff --git a/lldb/source/Host/common/FileAction.cpp b/lldb/source/Host/common/FileAction.cpp
index f980d3224640e0..e1c3e14a165ea9 100644
--- a/lldb/source/Host/common/FileAction.cpp
+++ b/lldb/source/Host/common/FileAction.cpp
@@ -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 {
diff --git a/lldb/unittests/Host/FileActionTest.cpp b/lldb/unittests/Host/FileActionTest.cpp
index b208169aac20e6..8c8b81a9250310 100644
--- a/lldb/unittests/Host/FileActionTest.cpp
+++ b/lldb/unittests/Host/FileActionTest.cpp
@@ -6,6 +6,8 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include <fcntl.h>
+
 #include "lldb/Host/FileAction.h"
 #include "gtest/gtest.h"
 
@@ -17,3 +19,25 @@ 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);
+}

github-actions · 2024-10-17T03:38:59Z

✅ With the latest revision this PR passed the C/C++ code formatter.

DavidSpickett · 2024-10-17T07:44:45Z

What you describe makes some sense, but could you give an example of where this is used?

Can this be triggered with lldb program file --<some option goes here>? If so it would be good to 1. put that in the commit message in case anyone hits the same problem and searches the log, and 2. add a release note.

labath · 2024-10-17T14:10:41Z

It would be nice to have a test which verifies actual behavior rather than just some flag being set somewhere. For example, you could add to/extend test/API/python_api/process/io/TestProcessIO.py so that it writes some initial data to the output file, and then verify that the data has disappeared.

kusmour · 2024-10-17T16:21:52Z

What you describe makes some sense, but could you give an example of where this is used?

Can this be triggered with lldb program file --<some option goes here>? If so it would be good to 1. put that in the commit message in case anyone hits the same problem and searches the log, and 2. add a release note.

For example, the settings target.output-path and target.error-path takes a path and lldb will call this function to write to the file.

Will add both later! Thanks!

kusmour · 2024-10-17T23:50:04Z

@DavidSpickett If you don't mind me asking, how do we add to release note? Thanks in advance!

bulbazord · 2024-10-25T17:32:32Z

lldb/unittests/Host/FileActionTest.cpp

@@ -6,6 +6,8 @@
 //
 //===----------------------------------------------------------------------===//

+#include <fcntl.h>


Will this work on windows?

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

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

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

LLDB windows build failure: https://lab.llvm.org/buildbot/#/builders/141/builds/3462

API test failed for remote platform in [#112657](#112657) Previously when putting files onto remote platform, I used `platform file write -d <data>` which actually required a `platform file open <path>` first in order to obtain a file descriptor. eg. in file [TestGDBRemotePlatformFile.py](https://github.com/llvm/llvm-project/blob/94e7d9c0bfe517507ea08b00fb00c32fb2837a82/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py#L24-L32) To fix this, use the `platform put-file` method, which is used in the `redirect_stdin` from this test already.

vvereschaka · 2024-10-30T21:38:05Z

@kusmour ,

The lldb-api::TestProcessIO.py test gets failed on the lldb-remote-linux builder with the following error:

...
AssertionError: False is not true : Command 'platform file write "/home/ubuntu/lldb-tests/python_api/process/io/6/TestProcessIO.test_stdout_stderr_redirection_to_existing_files/output.txt" -d "This content should be overwritten."' did not return successfully
Error output:
error: '"/home/ubuntu/lldb-tests/python_api/process/io/6/TestProcessIO.test_stdout_stderr_redirection_to_existing_files/output.txt"' is not a valid file descriptor.

see more details here: https://lab.llvm.org/buildbot/#/builders/195/builds/321

kusmour · 2024-10-30T21:40:32Z

@kusmour ,

The lldb-api::TestProcessIO.py test gets failed on the lldb-remote-linux builder with the following error:


...

AssertionError: False is not true : Command 'platform file write "/home/ubuntu/lldb-tests/python_api/process/io/6/TestProcessIO.test_stdout_stderr_redirection_to_existing_files/output.txt" -d "This content should be overwritten."' did not return successfully

Error output:

error: '"/home/ubuntu/lldb-tests/python_api/process/io/6/TestProcessIO.test_stdout_stderr_redirection_to_existing_files/output.txt"' is not a valid file descriptor.

see more details here: https://lab.llvm.org/buildbot/#/builders/195/builds/321

Hi thank you for your msg. I have the fix up. Merged and waiting on the test now: #114119

Disable part of the test failing on windows. as O_NOCTTY and O_RDONLY dont have same behavior on windows vs linux.

API test failed for remote platform in [llvm#112657](llvm#112657) Previously when putting files onto remote platform, I used `platform file write -d <data>` which actually required a `platform file open <path>` first in order to obtain a file descriptor. eg. in file [TestGDBRemotePlatformFile.py](https://github.com/llvm/llvm-project/blob/94e7d9c0bfe517507ea08b00fb00c32fb2837a82/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py#L24-L32) To fix this, use the `platform put-file` method, which is used in the `redirect_stdin` from this test already.

Disable part of the test failing on windows. as O_NOCTTY and O_RDONLY dont have same behavior on windows vs linux.

When `FileAction` opens file with write access, it doesn't clear the file nor append to the end of the file if it already exists. Instead, it writes from cursor index 0. For example, by using the settings `target.output-path` and `target.error-path`, lldb will redirect process stdout/stderr to files. It then calls this function to write to the files which the above symptoms appear. ## Test - Added unit test checking the file flags - Added 2 api tests checking - File content overwritten if the file path already exists - Stdout and stderr redirection to the same file doesn't change its behavior

LLDB windows build failure: https://lab.llvm.org/buildbot/#/builders/141/builds/3462

API test failed for remote platform in [llvm#112657](llvm#112657) Previously when putting files onto remote platform, I used `platform file write -d <data>` which actually required a `platform file open <path>` first in order to obtain a file descriptor. eg. in file [TestGDBRemotePlatformFile.py](https://github.com/llvm/llvm-project/blob/94e7d9c0bfe517507ea08b00fb00c32fb2837a82/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py#L24-L32) To fix this, use the `platform put-file` method, which is used in the `redirect_stdin` from this test already.

Disable part of the test failing on windows. as O_NOCTTY and O_RDONLY dont have same behavior on windows vs linux.

kusmour requested a review from JDevlieghere as a code owner October 17, 2024 03:35

llvmbot added the lldb label Oct 17, 2024

kusmour requested review from clayborg and jeffreytan81 October 17, 2024 03:35

kusmour force-pushed the lldb-file-action-open branch from eac4e0b to 48831c6 Compare October 17, 2024 23:32

kusmour force-pushed the lldb-file-action-open branch from 48831c6 to 4acbf06 Compare October 22, 2024 20:54

kusmour requested a review from bulbazord October 22, 2024 20:58

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

6ee1560

kusmour force-pushed the lldb-file-action-open branch from 4acbf06 to 6ee1560 Compare October 25, 2024 03:28

bulbazord reviewed Oct 25, 2024

View reviewed changes

bulbazord approved these changes Oct 29, 2024

View reviewed changes

kusmour merged commit efc6d33 into llvm:main Oct 29, 2024
9 checks passed

kusmour mentioned this pull request Oct 29, 2024

[lldb] Fix API test for file redirection to existing files #114119

Merged

ZequanWu added a commit to ZequanWu/llvm-project that referenced this pull request Oct 29, 2024

[lldb] Fix lldb windows build breakage from llvm#112657.

a325c53

LLDB windows build failure: https://lab.llvm.org/buildbot/#/builders/141/builds/3462

github-actions bot mentioned this pull request Oct 29, 2024

Update diff October 29th 2024, 10:26:10 pm dtcxzyw/llvm-opt-benchmark#1568

Closed

kusmour deleted the lldb-file-action-open branch October 30, 2024 20:58

omjavaid added a commit that referenced this pull request Oct 31, 2024

[lldb][test] Fix FileActionTest.cpp for Windows (#112657)

3bc58fc

Disable part of the test failing on windows. as O_NOCTTY and O_RDONLY dont have same behavior on windows vs linux.

smallp-o-p pushed a commit to smallp-o-p/llvm-project that referenced this pull request Nov 3, 2024

[lldb][test] Fix FileActionTest.cpp for Windows (llvm#112657)

f564be7

Disable part of the test failing on windows. as O_NOCTTY and O_RDONLY dont have same behavior on windows vs linux.

NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024

[lldb] Fix lldb windows build breakage from llvm#112657.

89bde35

LLDB windows build failure: https://lab.llvm.org/buildbot/#/builders/141/builds/3462

NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024

[lldb][test] Fix FileActionTest.cpp for Windows (llvm#112657)

36701f0

Disable part of the test failing on windows. as O_NOCTTY and O_RDONLY dont have same behavior on windows vs linux.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

kusmour commented Oct 17, 2024 •

edited

Loading

Uh oh!

llvmbot commented Oct 17, 2024

Uh oh!

github-actions bot commented Oct 17, 2024 •

edited

Loading

Uh oh!

DavidSpickett commented Oct 17, 2024

Uh oh!

labath commented Oct 17, 2024

Uh oh!

kusmour commented Oct 17, 2024

Uh oh!

kusmour commented Oct 17, 2024

Uh oh!

bulbazord Oct 25, 2024

Uh oh!

kusmour Oct 25, 2024 •

edited

Loading

Uh oh!

Jlalond Oct 29, 2024

Uh oh!

bulbazord Oct 29, 2024

Uh oh!

Uh oh!

vvereschaka commented Oct 30, 2024

Uh oh!

kusmour commented Oct 30, 2024

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

kusmour commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test

Uh oh!

llvmbot commented Oct 17, 2024

Uh oh!

github-actions bot commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DavidSpickett commented Oct 17, 2024

Uh oh!

labath commented Oct 17, 2024

Uh oh!

kusmour commented Oct 17, 2024

Uh oh!

kusmour commented Oct 17, 2024

Uh oh!

bulbazord Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

kusmour Oct 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jlalond Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

bulbazord Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vvereschaka commented Oct 30, 2024

Uh oh!

kusmour commented Oct 30, 2024

Uh oh!

Uh oh!

kusmour commented Oct 17, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading

kusmour Oct 25, 2024 •

edited

Loading