llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Ebuka Ezike (da-viper) <details> <summary>Changes</summary> Align the return value for all `query type` `GetCString` SB API. This API signature is similar to `size_t GetPath(char *path, size_t len)`. You query it first to get the amount of memory to allocate and a second time with the allocated memory. The issue is that the return value is different depending on what function you call. It is expected to return size of the string (including the NULL byte) or zero if the internal string is empty. every time regardless of the function input. Align SBFileSpec to match that. e.g. internal_string = "HELLO"; res = GetPath(buf, 10); // res = 6 , buf = "HELLO\0.." res = GetPath(buf, 2); // res = 6, buf = "H\0..." res = GetPaht(NULL, 0) // res = 6, buf = <unchanged> Add Unittest. --- Full diff: https://github.com/llvm/llvm-project/pull/220664.diff 7 Files Affected: - (modified) lldb/bindings/interfaces.swig (+1) - (modified) lldb/source/API/SBFileSpec.cpp (+15-7) - (modified) lldb/source/Utility/FileSpec.cpp (+13-4) - (modified) lldb/test/API/python_api/default-constructor/sb_filespec.py (+1) - (modified) lldb/test/API/python_api/filespec/TestFileSpecAPI.py (+1) - (modified) lldb/unittests/API/CMakeLists.txt (+1) - (added) lldb/unittests/API/SBFileSpecTest.cpp (+52) ``````````diff diff --git a/lldb/bindings/interfaces.swig b/lldb/bindings/interfaces.swig index 5c35d21e909fd..ec94be3686c00 100644 --- a/lldb/bindings/interfaces.swig +++ b/lldb/bindings/interfaces.swig @@ -93,6 +93,7 @@ /* Method replacements, must be called before the including API headers. */ REPLACE_BUF_GETTER_WITH_STRING(SBStructuredData, GetStringValue) +REPLACE_BUF_GETTER_WITH_STRING(SBFileSpec, GetPath) /* API headers */ %include "lldb/API/SBAddress.h" diff --git a/lldb/source/API/SBFileSpec.cpp b/lldb/source/API/SBFileSpec.cpp index d34cd06300744..9f5a10ca5830e 100644 --- a/lldb/source/API/SBFileSpec.cpp +++ b/lldb/source/API/SBFileSpec.cpp @@ -102,8 +102,20 @@ int SBFileSpec::ResolvePath(const char *src_path, char *dst_path, llvm::SmallString<64> result(src_path); FileSystem::Instance().Resolve(result); - ::snprintf(dst_path, dst_len, "%s", result.c_str()); - return std::min(dst_len - 1, result.size()); + if (result.empty()) { + if (dst_path && dst_len != 0) + *dst_path = '\0'; + return 0; + } + + const size_t needed_len = result.size() + 1; // for the NULL byte. + if (dst_path && dst_len != 0) { + const size_t min_len = std::min(needed_len, dst_len); + const size_t copy_len = min_len - 1; // exclude space for NULL byte. + std::memcpy(dst_path, result.data(), copy_len); + dst_path[copy_len] = '\0'; + } + return needed_len; } const char *SBFileSpec::GetFilename() const { @@ -141,11 +153,7 @@ void SBFileSpec::SetDirectory(const char *directory) { uint32_t SBFileSpec::GetPath(char *dst_path, size_t dst_len) const { LLDB_INSTRUMENT_VA(this, dst_path, dst_len); - uint32_t result = m_opaque_up->GetPath(dst_path, dst_len); - - if (result == 0 && dst_path && dst_len > 0) - *dst_path = '\0'; - return result; + return m_opaque_up->GetPath(dst_path, dst_len); } const lldb_private::FileSpec *SBFileSpec::operator->() const { diff --git a/lldb/source/Utility/FileSpec.cpp b/lldb/source/Utility/FileSpec.cpp index 0703adfbc8440..133d2d32a8c37 100644 --- a/lldb/source/Utility/FileSpec.cpp +++ b/lldb/source/Utility/FileSpec.cpp @@ -379,12 +379,21 @@ void FileSpec::ClearDirectory() { // directory and path are stored in separate string values. size_t FileSpec::GetPath(char *path, size_t path_max_len, bool denormalize) const { - if (!path) + std::string result = GetPath(denormalize); + if (result.empty()) { + if (path && path_max_len != 0) + *path = '\0'; return 0; + } - std::string result = GetPath(denormalize); - ::snprintf(path, path_max_len, "%s", result.c_str()); - return std::min(path_max_len - 1, result.length()); + const size_t needed_len = result.size() + 1; // for the NULL byte. + if (path && path_max_len != 0) { + const size_t min_len = std::min(needed_len, path_max_len); + const size_t copy_len = min_len - 1; // exclude space for NULL byte. + std::memcpy(path, result.data(), copy_len); + path[copy_len] = '\0'; + } + return needed_len; } std::string FileSpec::GetPath(bool denormalize) const { diff --git a/lldb/test/API/python_api/default-constructor/sb_filespec.py b/lldb/test/API/python_api/default-constructor/sb_filespec.py index 5dd78b1ace98f..56b1e3f0ac1c1 100644 --- a/lldb/test/API/python_api/default-constructor/sb_filespec.py +++ b/lldb/test/API/python_api/default-constructor/sb_filespec.py @@ -11,4 +11,5 @@ def fuzz_obj(obj): obj.GetFilename() obj.GetDirectory() obj.GetPath(1) + obj.GetPath() obj.GetDescription(lldb.SBStream()) diff --git a/lldb/test/API/python_api/filespec/TestFileSpecAPI.py b/lldb/test/API/python_api/filespec/TestFileSpecAPI.py index 48ceb9a981967..4d3b96d10a513 100644 --- a/lldb/test/API/python_api/filespec/TestFileSpecAPI.py +++ b/lldb/test/API/python_api/filespec/TestFileSpecAPI.py @@ -27,6 +27,7 @@ def test_filespec_eq_path(self): self.assertFalse(spec == "/a/c") self.assertFalse(spec != "/a/b") self.assertTrue(spec != "/a/c") + self.assertEqual(spec.GetPath(), "/a/b") def test_filespec_eq_other_type(self): """Test SBFileSpec equality with unsupported types returns False.""" diff --git a/lldb/unittests/API/CMakeLists.txt b/lldb/unittests/API/CMakeLists.txt index f6cf7fd37a6d2..100c9afbfafc5 100644 --- a/lldb/unittests/API/CMakeLists.txt +++ b/lldb/unittests/API/CMakeLists.txt @@ -4,6 +4,7 @@ add_lldb_unittest(APITests SBLineEntryTest.cpp SBMutexTest.cpp SBBreakpointClearConditionTest.cpp + SBFileSpecTest.cpp SBProtocolServerTest.cpp SBAPITEST diff --git a/lldb/unittests/API/SBFileSpecTest.cpp b/lldb/unittests/API/SBFileSpecTest.cpp new file mode 100644 index 0000000000000..2e66e6f1b57ab --- /dev/null +++ b/lldb/unittests/API/SBFileSpecTest.cpp @@ -0,0 +1,52 @@ +//===-- SBFileSpecTest.cpp ------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===/ + +// Use the umbrella header for -Wdocumentation. +#include "lldb/API/LLDB.h" + +#include "lldb/API/SBFileSpec.h" +#include "gtest/gtest.h" + +#include <cstring> +#include <string> + +TEST(SBFileSpecTest, GetPath) { + const std::string path = "/tmp/lldb-sbfilespec-test/file.txt"; + const size_t needed_len = path.size() + 1; // including NULL byte. + + lldb::SBFileSpec fs(path.c_str(), /*resolve=*/false); + ASSERT_TRUE(fs.IsValid()); + + // Verify large buffer returns needed_len and fills the buffer. + char buf[256]; + constexpr size_t buf_size = sizeof(buf); + std::memset(buf, 'X', buf_size); + + ASSERT_GE(buf_size, needed_len); + EXPECT_EQ(fs.GetPath(buf, buf_size), needed_len); + EXPECT_STREQ(buf, path.c_str()); + + // Verify querying path returns the size needed without writing. + EXPECT_EQ(fs.GetPath(nullptr, 0), needed_len); + + // Verify smaller buffer returns the full needed size (including the null + // byte) and the buffer is truncated and NUL-terminated. + char small_buf[8]; + constexpr size_t small_buf_size = sizeof(small_buf); + std::memset(small_buf, 'X', small_buf_size); + EXPECT_EQ(fs.GetPath(small_buf, small_buf_size), needed_len); + EXPECT_EQ(small_buf[small_buf_size - 1], '\0'); + EXPECT_EQ(std::strncmp(small_buf, path.c_str(), small_buf_size - 1), 0); + + // Verify empty filespec returns 0 and NUL-terminates the buffer. + lldb::SBFileSpec empty_fs; + char empty_buf[16]; + std::memset(empty_buf, 'X', sizeof(empty_buf)); + EXPECT_EQ(empty_fs.GetPath(empty_buf, sizeof(empty_buf)), 0U); + EXPECT_EQ(empty_buf[0], '\0'); +} `````````` </details> https://github.com/llvm/llvm-project/pull/220664 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
