[Lldb-commits] [lldb] r330568 - [CMake] Gate 'dsymutil' dependency
Author: jdevlieghere Date: Mon Apr 23 01:44:06 2018 New Revision: 330568 URL: http://llvm.org/viewvc/llvm-project?rev=330568&view=rev Log: [CMake] Gate 'dsymutil' dependency Only add `dsymutil` as a test dependency when this is not a stand-alone build (LLDB_BUILT_STANDALONE). Modified: lldb/trunk/test/CMakeLists.txt Modified: lldb/trunk/test/CMakeLists.txt URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/test/CMakeLists.txt?rev=330568&r1=330567&r2=330568&view=diff == --- lldb/trunk/test/CMakeLists.txt (original) +++ lldb/trunk/test/CMakeLists.txt Mon Apr 23 01:44:06 2018 @@ -13,7 +13,7 @@ function(add_python_test_target name tes ) endfunction() -set(LLDB_TEST_DEPS lldb dsymutil) +set(LLDB_TEST_DEPS lldb) # darwin-debug is an hard dependency for the testsuite. if (CMAKE_SYSTEM_NAME MATCHES "Darwin") @@ -35,7 +35,7 @@ if(TARGET lldb-mi) endif() if(NOT LLDB_BUILT_STANDALONE) - list(APPEND LLDB_TEST_DEPS yaml2obj) + list(APPEND LLDB_TEST_DEPS yaml2obj dsymutil) endif() if(TARGET liblldb) ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r330578 - llgs-tests: Increase packet time out
Author: labath Date: Mon Apr 23 04:22:44 2018 New Revision: 330578 URL: http://llvm.org/viewvc/llvm-project?rev=330578&view=rev Log: llgs-tests: Increase packet time out The default packet timeout of 1 second is a bit too small for these tests, particularly as they are working in ack-mode, which means they need to fit twice as many packets into the timeslot. This does not seem to be a problem on the bots, but for some people these tests are timing out regularly. I can't be sure increasing this will solve their problem, but this does seem like a likely culprit. Modified: lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp Modified: lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp?rev=330578&r1=330577&r2=330578&view=diff == --- lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp (original) +++ lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp Mon Apr 23 04:22:44 2018 @@ -29,6 +29,7 @@ using namespace llgs_tests; TestClient::TestClient(std::unique_ptr Conn) { SetConnection(Conn.release()); + SetPacketTimeout(std::chrono::seconds(10)); SendAck(); // Send this as a handshake. } ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45949: [dotest] Make the set of tests independent of the test configuration
labath created this revision. labath added reviewers: JDevlieghere, aprantl. Herald added a subscriber: eraman. In the magic test duplicator, we were making the decision whether to create a test variant based on the compiler and the target platform. This meant that the set of known tests was different for each test configuration. This patch makes the set of generated test variants static and handles the skipping via runtime checks instead. This is more consistent with how we do other test-skipping decision (e.g. for libc++ tests), and makes it easier to expose the full set of tests to lit, which now does not need to know anything about what things can potentially cause tests to appear or disappear. https://reviews.llvm.org/D45949 Files: packages/Python/lldbsuite/test/dotest.py packages/Python/lldbsuite/test/lldbinline.py packages/Python/lldbsuite/test/lldbtest.py Index: packages/Python/lldbsuite/test/lldbtest.py === --- packages/Python/lldbsuite/test/lldbtest.py +++ packages/Python/lldbsuite/test/lldbtest.py @@ -1736,65 +1736,29 @@ for attrname, attrvalue in attrs.items(): if attrname.startswith("test") and not getattr( attrvalue, "__no_debug_info_test__", False): -target_platform = lldb.DBG.GetSelectedPlatform( -).GetTriple().split('-')[2] # If any debug info categories were explicitly tagged, assume that list to be # authoritative. If none were specified, try with all debug # info formats. -all_dbginfo_categories = set( -test_categories.debug_info_categories) - set(configuration.skipCategories) +all_dbginfo_categories = set(test_categories.debug_info_categories) categories = set( getattr( attrvalue, "categories", [])) & all_dbginfo_categories if not categories: categories = all_dbginfo_categories -supported_categories = [ -x for x in categories if test_categories.is_supported_on_platform( -x, target_platform, configuration.compiler)] - -if "dsym" in supported_categories: -@decorators.add_test_categories(["dsym"]) +for cat in categories: +@decorators.add_test_categories([cat]) @wraps(attrvalue) -def dsym_test_method(self, attrvalue=attrvalue): +def test_method(self, attrvalue=attrvalue): return attrvalue(self) -dsym_method_name = attrname + "_dsym" -dsym_test_method.__name__ = dsym_method_name -dsym_test_method.debug_info = "dsym" -newattrs[dsym_method_name] = dsym_test_method -if "dwarf" in supported_categories: -@decorators.add_test_categories(["dwarf"]) -@wraps(attrvalue) -def dwarf_test_method(self, attrvalue=attrvalue): -return attrvalue(self) -dwarf_method_name = attrname + "_dwarf" -dwarf_test_method.__name__ = dwarf_method_name -dwarf_test_method.debug_info = "dwarf" -newattrs[dwarf_method_name] = dwarf_test_method - -if "dwo" in supported_categories: -@decorators.add_test_categories(["dwo"]) -@wraps(attrvalue) -def dwo_test_method(self, attrvalue=attrvalue): -return attrvalue(self) -dwo_method_name = attrname + "_dwo" -dwo_test_method.__name__ = dwo_method_name -dwo_test_method.debug_info = "dwo" -newattrs[dwo_method_name] = dwo_test_method - -if "gmodules" in supported_categories: -@decorators.add_test_categories(["gmodules"]) -@wraps(attrvalue) -def gmodules_test_method(self, attrvalue=attrvalue): -return attrvalue(self) -gmodules_method_name = attrname + "_gmodules" -gmodules_test_method.__name__ = gmodules_method_name -gmodules_test_method.debug_info = "gmodules" -newattrs[gmodules_method_name] = gmodules_test_method +method_name = attrname + "_" + cat +test_method.__name__ = method_name +test_method.debug_info = cat +newattrs[method_name] = test_method else: newattrs[attrname] = attrvalue Index: packages/Python/lldbsuite/test/lldbinline.py
[Lldb-commits] [lldb] r330617 - [CMake] Add the missing `dsymutil` dependency when running tests.
Author: davide Date: Mon Apr 23 10:06:55 2018 New Revision: 330617 URL: http://llvm.org/viewvc/llvm-project?rev=330617&view=rev Log: [CMake] Add the missing `dsymutil` dependency when running tests. Modified: lldb/trunk/lit/CMakeLists.txt Modified: lldb/trunk/lit/CMakeLists.txt URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/CMakeLists.txt?rev=330617&r1=330616&r2=330617&view=diff == --- lldb/trunk/lit/CMakeLists.txt (original) +++ lldb/trunk/lit/CMakeLists.txt Mon Apr 23 10:06:55 2018 @@ -38,6 +38,7 @@ configure_lit_site_cfg( set(LLDB_TEST_DEPS LLDBUnitTests + dsymutil lldb lldb-test llvm-config ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.
clayborg created this revision. clayborg added reviewers: labath, jingham, davide, pranavb. Herald added subscribers: JDevlieghere, aprantl. Always normalizing lldb_private::FileSpec paths will help us get a consistent results from comparisons when setting breakpoints and when looking for source files. This also removes a lot of complexity from the comparison routines. Modified the DWARF line table parser to use the normalized compile unit directory if needed. https://reviews.llvm.org/D45977 Files: include/lldb/Core/FileSpecList.h include/lldb/Utility/FileSpec.h lldb.xcodeproj/xcshareddata/xcschemes/darwin-debug.xcscheme lldb.xcodeproj/xcshareddata/xcschemes/desktop.xcscheme lldb.xcodeproj/xcshareddata/xcschemes/lldb-gtest.xcscheme source/Breakpoint/BreakpointResolverFileLine.cpp source/Core/FileSpecList.cpp source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp source/Plugins/SymbolFile/DWARF/DWARFDebugLine.h source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp source/Symbol/CompileUnit.cpp source/Symbol/Declaration.cpp source/Utility/FileSpec.cpp unittests/Utility/FileSpecTest.cpp Index: unittests/Utility/FileSpecTest.cpp === --- unittests/Utility/FileSpecTest.cpp +++ unittests/Utility/FileSpecTest.cpp @@ -54,17 +54,14 @@ FileSpec fs_posix_trailing_slash("/foo/bar/", false, FileSpec::ePathSyntaxPosix); - EXPECT_STREQ("/foo/bar/.", fs_posix_trailing_slash.GetCString()); - EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetDirectory().GetCString()); - EXPECT_STREQ(".", fs_posix_trailing_slash.GetFilename().GetCString()); + EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetCString()); + EXPECT_STREQ("/foo", fs_posix_trailing_slash.GetDirectory().GetCString()); + EXPECT_STREQ("bar", fs_posix_trailing_slash.GetFilename().GetCString()); FileSpec fs_windows_trailing_slash("F:\\bar\\", false, FileSpec::ePathSyntaxWindows); - EXPECT_STREQ("F:\\bar\\.", fs_windows_trailing_slash.GetCString()); - // EXPECT_STREQ("F:\\bar", - // fs_windows_trailing_slash.GetDirectory().GetCString()); // It returns - // "F:/bar" - EXPECT_STREQ(".", fs_windows_trailing_slash.GetFilename().GetCString()); + EXPECT_STREQ("F:\\bar", fs_windows_trailing_slash.GetCString()); + EXPECT_STREQ("bar", fs_windows_trailing_slash.GetFilename().GetCString()); } TEST(FileSpecTest, AppendPathComponent) { @@ -131,32 +128,13 @@ EXPECT_STREQ("F:\\bar", fs_windows_root.GetCString()); } -static void Compare(const FileSpec &one, const FileSpec &two, bool full_match, -bool remove_backup_dots, bool result) { - EXPECT_EQ(result, FileSpec::Equal(one, two, full_match, remove_backup_dots)) - << "File one: " << one.GetCString() << "\nFile two: " << two.GetCString() - << "\nFull match: " << full_match - << "\nRemove backup dots: " << remove_backup_dots; -} - TEST(FileSpecTest, EqualSeparator) { FileSpec backward("C:\\foo\\bar", false, FileSpec::ePathSyntaxWindows); FileSpec forward("C:/foo/bar", false, FileSpec::ePathSyntaxWindows); EXPECT_EQ(forward, backward); - - const bool full_match = true; - const bool remove_backup_dots = true; - const bool match = true; - Compare(forward, backward, full_match, remove_backup_dots, match); - Compare(forward, backward, full_match, !remove_backup_dots, match); - Compare(forward, backward, !full_match, remove_backup_dots, match); - Compare(forward, backward, !full_match, !remove_backup_dots, match); } TEST(FileSpecTest, EqualDotsWindows) { - const bool full_match = true; - const bool remove_backup_dots = true; - const bool match = true; std::pair tests[] = { {R"(C:\foo\bar\baz)", R"(C:\foo\foo\..\bar\baz)"}, {R"(C:\bar\baz)", R"(C:\foo\..\bar\baz)"}, @@ -170,18 +148,11 @@ for (const auto &test : tests) { FileSpec one(test.first, false, FileSpec::ePathSyntaxWindows); FileSpec two(test.second, false, FileSpec::ePathSyntaxWindows); -EXPECT_NE(one, two); -Compare(one, two, full_match, remove_backup_dots, match); -Compare(one, two, full_match, !remove_backup_dots, !match); -Compare(one, two, !full_match, remove_backup_dots, match); -Compare(one, two, !full_match, !remove_backup_dots, !match); +EXPECT_EQ(one, two); } } TEST(FileSpecTest, EqualDotsPosix) { - const bool full_match = true; - const bool remove_backup_dots = true; - const bool match = true; std::pair tests[] = { {R"(/foo/bar/baz)", R"(/foo/foo/../bar/baz)"}, {R"(/bar/baz)", R"(/foo/../bar/baz)"}, @@ -193,18 +164,11 @@ for (const auto &test : tests) { FileSpec one(test.first, false, FileSpec::ePathSyntaxPosix); FileSpec two(test.second, false, FileSpec::ePathSyntaxPosix); -EXPECT_NE(one, two); -Compare(one, two, full_match, remove_backup_dots, match); -Co
[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.
clayborg updated this revision to Diff 143608. clayborg added a comment. Remove unwanted file changes. https://reviews.llvm.org/D45977 Files: include/lldb/Core/FileSpecList.h include/lldb/Utility/FileSpec.h source/Breakpoint/BreakpointResolverFileLine.cpp source/Core/FileSpecList.cpp source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp source/Plugins/SymbolFile/DWARF/DWARFDebugLine.h source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp source/Symbol/CompileUnit.cpp source/Symbol/Declaration.cpp source/Utility/FileSpec.cpp unittests/Utility/FileSpecTest.cpp Index: unittests/Utility/FileSpecTest.cpp === --- unittests/Utility/FileSpecTest.cpp +++ unittests/Utility/FileSpecTest.cpp @@ -54,17 +54,14 @@ FileSpec fs_posix_trailing_slash("/foo/bar/", false, FileSpec::ePathSyntaxPosix); - EXPECT_STREQ("/foo/bar/.", fs_posix_trailing_slash.GetCString()); - EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetDirectory().GetCString()); - EXPECT_STREQ(".", fs_posix_trailing_slash.GetFilename().GetCString()); + EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetCString()); + EXPECT_STREQ("/foo", fs_posix_trailing_slash.GetDirectory().GetCString()); + EXPECT_STREQ("bar", fs_posix_trailing_slash.GetFilename().GetCString()); FileSpec fs_windows_trailing_slash("F:\\bar\\", false, FileSpec::ePathSyntaxWindows); - EXPECT_STREQ("F:\\bar\\.", fs_windows_trailing_slash.GetCString()); - // EXPECT_STREQ("F:\\bar", - // fs_windows_trailing_slash.GetDirectory().GetCString()); // It returns - // "F:/bar" - EXPECT_STREQ(".", fs_windows_trailing_slash.GetFilename().GetCString()); + EXPECT_STREQ("F:\\bar", fs_windows_trailing_slash.GetCString()); + EXPECT_STREQ("bar", fs_windows_trailing_slash.GetFilename().GetCString()); } TEST(FileSpecTest, AppendPathComponent) { @@ -131,32 +128,13 @@ EXPECT_STREQ("F:\\bar", fs_windows_root.GetCString()); } -static void Compare(const FileSpec &one, const FileSpec &two, bool full_match, -bool remove_backup_dots, bool result) { - EXPECT_EQ(result, FileSpec::Equal(one, two, full_match, remove_backup_dots)) - << "File one: " << one.GetCString() << "\nFile two: " << two.GetCString() - << "\nFull match: " << full_match - << "\nRemove backup dots: " << remove_backup_dots; -} - TEST(FileSpecTest, EqualSeparator) { FileSpec backward("C:\\foo\\bar", false, FileSpec::ePathSyntaxWindows); FileSpec forward("C:/foo/bar", false, FileSpec::ePathSyntaxWindows); EXPECT_EQ(forward, backward); - - const bool full_match = true; - const bool remove_backup_dots = true; - const bool match = true; - Compare(forward, backward, full_match, remove_backup_dots, match); - Compare(forward, backward, full_match, !remove_backup_dots, match); - Compare(forward, backward, !full_match, remove_backup_dots, match); - Compare(forward, backward, !full_match, !remove_backup_dots, match); } TEST(FileSpecTest, EqualDotsWindows) { - const bool full_match = true; - const bool remove_backup_dots = true; - const bool match = true; std::pair tests[] = { {R"(C:\foo\bar\baz)", R"(C:\foo\foo\..\bar\baz)"}, {R"(C:\bar\baz)", R"(C:\foo\..\bar\baz)"}, @@ -170,18 +148,11 @@ for (const auto &test : tests) { FileSpec one(test.first, false, FileSpec::ePathSyntaxWindows); FileSpec two(test.second, false, FileSpec::ePathSyntaxWindows); -EXPECT_NE(one, two); -Compare(one, two, full_match, remove_backup_dots, match); -Compare(one, two, full_match, !remove_backup_dots, !match); -Compare(one, two, !full_match, remove_backup_dots, match); -Compare(one, two, !full_match, !remove_backup_dots, !match); +EXPECT_EQ(one, two); } } TEST(FileSpecTest, EqualDotsPosix) { - const bool full_match = true; - const bool remove_backup_dots = true; - const bool match = true; std::pair tests[] = { {R"(/foo/bar/baz)", R"(/foo/foo/../bar/baz)"}, {R"(/bar/baz)", R"(/foo/../bar/baz)"}, @@ -193,18 +164,11 @@ for (const auto &test : tests) { FileSpec one(test.first, false, FileSpec::ePathSyntaxPosix); FileSpec two(test.second, false, FileSpec::ePathSyntaxPosix); -EXPECT_NE(one, two); -Compare(one, two, full_match, remove_backup_dots, match); -Compare(one, two, full_match, !remove_backup_dots, !match); -Compare(one, two, !full_match, remove_backup_dots, match); -Compare(one, two, !full_match, !remove_backup_dots, !match); +EXPECT_EQ(one, two); } } TEST(FileSpecTest, EqualDotsPosixRoot) { - const bool full_match = true; - const bool remove_backup_dots = true; - const bool match = true; std::pair tests[] = { {R"(/)", R"(/..)"}, {R"(/)", R"(/.)"}, @@ -214,11 +178,7 @@ for (const auto &test : tests) { FileSpec one(test.first, false, FileS
[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.
aprantl added inline comments. Comment at: lldb.xcodeproj/xcshareddata/xcschemes/darwin-debug.xcscheme:29 selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.GDB" + language = "" shouldUseLaunchSchemeArgsEnv = "YES"> Is this an accidental change or is it relevant to the patch? Comment at: lldb.xcodeproj/xcshareddata/xcschemes/desktop.xcscheme:68 + This looks like an accidental commit? https://reviews.llvm.org/D45977 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.
clayborg updated this revision to Diff 143609. clayborg added a comment. Remove commented out code. https://reviews.llvm.org/D45977 Files: include/lldb/Core/FileSpecList.h include/lldb/Utility/FileSpec.h source/Breakpoint/BreakpointResolverFileLine.cpp source/Core/FileSpecList.cpp source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp source/Plugins/SymbolFile/DWARF/DWARFDebugLine.h source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp source/Symbol/CompileUnit.cpp source/Symbol/Declaration.cpp source/Utility/FileSpec.cpp unittests/Utility/FileSpecTest.cpp Index: unittests/Utility/FileSpecTest.cpp === --- unittests/Utility/FileSpecTest.cpp +++ unittests/Utility/FileSpecTest.cpp @@ -54,17 +54,14 @@ FileSpec fs_posix_trailing_slash("/foo/bar/", false, FileSpec::ePathSyntaxPosix); - EXPECT_STREQ("/foo/bar/.", fs_posix_trailing_slash.GetCString()); - EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetDirectory().GetCString()); - EXPECT_STREQ(".", fs_posix_trailing_slash.GetFilename().GetCString()); + EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetCString()); + EXPECT_STREQ("/foo", fs_posix_trailing_slash.GetDirectory().GetCString()); + EXPECT_STREQ("bar", fs_posix_trailing_slash.GetFilename().GetCString()); FileSpec fs_windows_trailing_slash("F:\\bar\\", false, FileSpec::ePathSyntaxWindows); - EXPECT_STREQ("F:\\bar\\.", fs_windows_trailing_slash.GetCString()); - // EXPECT_STREQ("F:\\bar", - // fs_windows_trailing_slash.GetDirectory().GetCString()); // It returns - // "F:/bar" - EXPECT_STREQ(".", fs_windows_trailing_slash.GetFilename().GetCString()); + EXPECT_STREQ("F:\\bar", fs_windows_trailing_slash.GetCString()); + EXPECT_STREQ("bar", fs_windows_trailing_slash.GetFilename().GetCString()); } TEST(FileSpecTest, AppendPathComponent) { @@ -131,32 +128,13 @@ EXPECT_STREQ("F:\\bar", fs_windows_root.GetCString()); } -static void Compare(const FileSpec &one, const FileSpec &two, bool full_match, -bool remove_backup_dots, bool result) { - EXPECT_EQ(result, FileSpec::Equal(one, two, full_match, remove_backup_dots)) - << "File one: " << one.GetCString() << "\nFile two: " << two.GetCString() - << "\nFull match: " << full_match - << "\nRemove backup dots: " << remove_backup_dots; -} - TEST(FileSpecTest, EqualSeparator) { FileSpec backward("C:\\foo\\bar", false, FileSpec::ePathSyntaxWindows); FileSpec forward("C:/foo/bar", false, FileSpec::ePathSyntaxWindows); EXPECT_EQ(forward, backward); - - const bool full_match = true; - const bool remove_backup_dots = true; - const bool match = true; - Compare(forward, backward, full_match, remove_backup_dots, match); - Compare(forward, backward, full_match, !remove_backup_dots, match); - Compare(forward, backward, !full_match, remove_backup_dots, match); - Compare(forward, backward, !full_match, !remove_backup_dots, match); } TEST(FileSpecTest, EqualDotsWindows) { - const bool full_match = true; - const bool remove_backup_dots = true; - const bool match = true; std::pair tests[] = { {R"(C:\foo\bar\baz)", R"(C:\foo\foo\..\bar\baz)"}, {R"(C:\bar\baz)", R"(C:\foo\..\bar\baz)"}, @@ -170,18 +148,11 @@ for (const auto &test : tests) { FileSpec one(test.first, false, FileSpec::ePathSyntaxWindows); FileSpec two(test.second, false, FileSpec::ePathSyntaxWindows); -EXPECT_NE(one, two); -Compare(one, two, full_match, remove_backup_dots, match); -Compare(one, two, full_match, !remove_backup_dots, !match); -Compare(one, two, !full_match, remove_backup_dots, match); -Compare(one, two, !full_match, !remove_backup_dots, !match); +EXPECT_EQ(one, two); } } TEST(FileSpecTest, EqualDotsPosix) { - const bool full_match = true; - const bool remove_backup_dots = true; - const bool match = true; std::pair tests[] = { {R"(/foo/bar/baz)", R"(/foo/foo/../bar/baz)"}, {R"(/bar/baz)", R"(/foo/../bar/baz)"}, @@ -193,18 +164,11 @@ for (const auto &test : tests) { FileSpec one(test.first, false, FileSpec::ePathSyntaxPosix); FileSpec two(test.second, false, FileSpec::ePathSyntaxPosix); -EXPECT_NE(one, two); -Compare(one, two, full_match, remove_backup_dots, match); -Compare(one, two, full_match, !remove_backup_dots, !match); -Compare(one, two, !full_match, remove_backup_dots, match); -Compare(one, two, !full_match, !remove_backup_dots, !match); +EXPECT_EQ(one, two); } } TEST(FileSpecTest, EqualDotsPosixRoot) { - const bool full_match = true; - const bool remove_backup_dots = true; - const bool match = true; std::pair tests[] = { {R"(/)", R"(/..)"}, {R"(/)", R"(/.)"}, @@ -214,11 +178,7 @@ for (const auto &test : tests) { FileSpec one(test.first, false, FileSpec
[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.
aprantl added inline comments. Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:189 + // Always normalize our compile unit directory to get rid of redundant + // slashes and other path anonalies before we use it for path prepending + FileSpec local_spec(local_path, false); `// slashes and other path ano*m*alies before we use it for path prepending*.*` Comment at: source/Utility/FileSpec.cpp:105 +//-- +bool needsNormalization(const llvm::StringRef &path) { + for (auto i = path.find('/'); i != llvm::StringRef::npos; Are we sure that we need this? Could we use llvm::sys::path::has_relative_path & friends instead? http://llvm.org/doxygen/namespacellvm_1_1sys_1_1path.html Comment at: source/Utility/FileSpec.cpp:107 + for (auto i = path.find('/'); i != llvm::StringRef::npos; + i = path.find('/', i + 1)) { +const auto nextChar = safeCharAtIndex(path, i+1); shouldn't this be a more general path separator (we probably have something like this in Support/Path.h) https://reviews.llvm.org/D45977 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.
clayborg added inline comments. Comment at: source/Utility/FileSpec.cpp:105 +//-- +bool needsNormalization(const llvm::StringRef &path) { + for (auto i = path.find('/'); i != llvm::StringRef::npos; I only want to scan the string one time. This will return true if this contains relative redundancies or extra slashes, not if the path is relative. "./foo" doesn't need normalization and this will return false, but "foo/." would need to be normalized to "foo", as well as "foo/./bar" and "foo/../bar". So this function is doing something completely different than the LLVM counterparts Comment at: source/Utility/FileSpec.cpp:107 + for (auto i = path.find('/'); i != llvm::StringRef::npos; + i = path.find('/', i + 1)) { +const auto nextChar = safeCharAtIndex(path, i+1); no as the only caller to this function switches the slashes to '/' already.. https://reviews.llvm.org/D45977 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.
clayborg updated this revision to Diff 143643. clayborg added a comment. Fix comment typo https://reviews.llvm.org/D45977 Files: include/lldb/Core/FileSpecList.h include/lldb/Utility/FileSpec.h source/Breakpoint/BreakpointResolverFileLine.cpp source/Core/FileSpecList.cpp source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp source/Plugins/SymbolFile/DWARF/DWARFDebugLine.h source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp source/Symbol/CompileUnit.cpp source/Symbol/Declaration.cpp source/Utility/FileSpec.cpp unittests/Utility/FileSpecTest.cpp Index: unittests/Utility/FileSpecTest.cpp === --- unittests/Utility/FileSpecTest.cpp +++ unittests/Utility/FileSpecTest.cpp @@ -54,17 +54,14 @@ FileSpec fs_posix_trailing_slash("/foo/bar/", false, FileSpec::ePathSyntaxPosix); - EXPECT_STREQ("/foo/bar/.", fs_posix_trailing_slash.GetCString()); - EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetDirectory().GetCString()); - EXPECT_STREQ(".", fs_posix_trailing_slash.GetFilename().GetCString()); + EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetCString()); + EXPECT_STREQ("/foo", fs_posix_trailing_slash.GetDirectory().GetCString()); + EXPECT_STREQ("bar", fs_posix_trailing_slash.GetFilename().GetCString()); FileSpec fs_windows_trailing_slash("F:\\bar\\", false, FileSpec::ePathSyntaxWindows); - EXPECT_STREQ("F:\\bar\\.", fs_windows_trailing_slash.GetCString()); - // EXPECT_STREQ("F:\\bar", - // fs_windows_trailing_slash.GetDirectory().GetCString()); // It returns - // "F:/bar" - EXPECT_STREQ(".", fs_windows_trailing_slash.GetFilename().GetCString()); + EXPECT_STREQ("F:\\bar", fs_windows_trailing_slash.GetCString()); + EXPECT_STREQ("bar", fs_windows_trailing_slash.GetFilename().GetCString()); } TEST(FileSpecTest, AppendPathComponent) { @@ -131,32 +128,13 @@ EXPECT_STREQ("F:\\bar", fs_windows_root.GetCString()); } -static void Compare(const FileSpec &one, const FileSpec &two, bool full_match, -bool remove_backup_dots, bool result) { - EXPECT_EQ(result, FileSpec::Equal(one, two, full_match, remove_backup_dots)) - << "File one: " << one.GetCString() << "\nFile two: " << two.GetCString() - << "\nFull match: " << full_match - << "\nRemove backup dots: " << remove_backup_dots; -} - TEST(FileSpecTest, EqualSeparator) { FileSpec backward("C:\\foo\\bar", false, FileSpec::ePathSyntaxWindows); FileSpec forward("C:/foo/bar", false, FileSpec::ePathSyntaxWindows); EXPECT_EQ(forward, backward); - - const bool full_match = true; - const bool remove_backup_dots = true; - const bool match = true; - Compare(forward, backward, full_match, remove_backup_dots, match); - Compare(forward, backward, full_match, !remove_backup_dots, match); - Compare(forward, backward, !full_match, remove_backup_dots, match); - Compare(forward, backward, !full_match, !remove_backup_dots, match); } TEST(FileSpecTest, EqualDotsWindows) { - const bool full_match = true; - const bool remove_backup_dots = true; - const bool match = true; std::pair tests[] = { {R"(C:\foo\bar\baz)", R"(C:\foo\foo\..\bar\baz)"}, {R"(C:\bar\baz)", R"(C:\foo\..\bar\baz)"}, @@ -170,18 +148,11 @@ for (const auto &test : tests) { FileSpec one(test.first, false, FileSpec::ePathSyntaxWindows); FileSpec two(test.second, false, FileSpec::ePathSyntaxWindows); -EXPECT_NE(one, two); -Compare(one, two, full_match, remove_backup_dots, match); -Compare(one, two, full_match, !remove_backup_dots, !match); -Compare(one, two, !full_match, remove_backup_dots, match); -Compare(one, two, !full_match, !remove_backup_dots, !match); +EXPECT_EQ(one, two); } } TEST(FileSpecTest, EqualDotsPosix) { - const bool full_match = true; - const bool remove_backup_dots = true; - const bool match = true; std::pair tests[] = { {R"(/foo/bar/baz)", R"(/foo/foo/../bar/baz)"}, {R"(/bar/baz)", R"(/foo/../bar/baz)"}, @@ -193,18 +164,11 @@ for (const auto &test : tests) { FileSpec one(test.first, false, FileSpec::ePathSyntaxPosix); FileSpec two(test.second, false, FileSpec::ePathSyntaxPosix); -EXPECT_NE(one, two); -Compare(one, two, full_match, remove_backup_dots, match); -Compare(one, two, full_match, !remove_backup_dots, !match); -Compare(one, two, !full_match, remove_backup_dots, match); -Compare(one, two, !full_match, !remove_backup_dots, !match); +EXPECT_EQ(one, two); } } TEST(FileSpecTest, EqualDotsPosixRoot) { - const bool full_match = true; - const bool remove_backup_dots = true; - const bool match = true; std::pair tests[] = { {R"(/)", R"(/..)"}, {R"(/)", R"(/.)"}, @@ -214,11 +178,7 @@ for (const auto &test : tests) { FileSpec one(test.first, false, FileSpec::ePathSyn
[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.
aprantl added inline comments. Comment at: source/Utility/FileSpec.cpp:107 + for (auto i = path.find('/'); i != llvm::StringRef::npos; + i = path.find('/', i + 1)) { +const auto nextChar = safeCharAtIndex(path, i+1); clayborg wrote: > no as the only caller to this function switches the slashes to '/' already.. If we change that to also replace double-`/` with singles, we could replace this function with a call to llvm::sys::path::remove_dots() http://llvm.org/doxygen/namespacellvm_1_1sys_1_1path.html#a35c103b5fb70a66a1cb5da3b56f588a1 Comment at: source/Utility/FileSpec.cpp:115 + case '/': +// "//" in the middle of a path needs to be normalized +if (i > 0) Does this also turn "//WORKGROUP/Foo" into "/WORKGROUP/Foo/"? https://reviews.llvm.org/D45977 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.
aprantl added a comment. One general question: why is this form of normalization preferred over calling realpath? https://reviews.llvm.org/D45977 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.
clayborg added inline comments. Comment at: source/Utility/FileSpec.cpp:107 + for (auto i = path.find('/'); i != llvm::StringRef::npos; + i = path.find('/', i + 1)) { +const auto nextChar = safeCharAtIndex(path, i+1); I am fine switching to using the llvm functions for removing, this is only detecting if any normalization needs to happen. If there is an equivalent LLVM function that will only run through the string one time to detect all needs for normalization (no multiple passes looking for "..", then for "." etc like we used to have), I would be happy to use it, but there doesn't seem to be. We are trying to avoid having to create a "SmallVectorImpl< char >" for all paths if they don't need fixing here. This function is just iterating through an llvm::StringRef just to see if normalization needs to happen. If it does, then we make a SmallVectorImpl< char > and we fix the path. We can easily use llvm functions for the fixing part. Comment at: source/Utility/FileSpec.cpp:115 + case '/': +// "//" in the middle of a path needs to be normalized +if (i > 0) aprantl wrote: > Does this also turn "//WORKGROUP/Foo" into "/WORKGROUP/Foo/"? No it does not,. Notice the "if (i > 0)" below. We need to keep leading "//" https://reviews.llvm.org/D45977 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.
clayborg added a comment. In https://reviews.llvm.org/D45977#1076048, @aprantl wrote: > One general question: why is this form of normalization preferred over > calling realpath? Normalization is everything we can do to fix up a path without knowing anything about the current working directory or any symlinks. So we can remove redundant references to the current directory (".") or the parent directory (".."), but only if they are not at the start of the path. Since our path may have been created on a different machine with an unknown symlinks, realpath really can't do anything for us. What is a path like "./foo.txt" relative to when stored in the debug info with no compilation directory? what if we have "/tmp/mysymlink" mean on another machine? We can't realpath it because we don't know the actual root or any of the symlinks. https://reviews.llvm.org/D45977 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.
aprantl added inline comments. Comment at: source/Utility/FileSpec.cpp:107 + for (auto i = path.find('/'); i != llvm::StringRef::npos; + i = path.find('/', i + 1)) { +const auto nextChar = safeCharAtIndex(path, i+1); clayborg wrote: > I am fine switching to using the llvm functions for removing, this is only > detecting if any normalization needs to happen. If there is an equivalent > LLVM function that will only run through the string one time to detect all > needs for normalization (no multiple passes looking for "..", then for "." > etc like we used to have), I would be happy to use it, but there doesn't seem > to be. We are trying to avoid having to create a "SmallVectorImpl< char >" > for all paths if they don't need fixing here. This function is just iterating > through an llvm::StringRef just to see if normalization needs to happen. If > it does, then we make a SmallVectorImpl< char > and we fix the path. We can > easily use llvm functions for the fixing part. I see, you want to avoid copying the string when it isn't necessary to do so. IMHO the best way to do this is to add a `bool hasDots(const Twine &)` function to llvm::sys::path and add a `bool remove_dot_dot` parameter to `removeDots()`. Since you have already written the unittests, adding the function to LLVM shouldn't be any extra work (I'll help reviewing), and we'll have 100 LoC less to maintain. This way we can avoid having two functions that perform path normalization that superficially look like they might do the same thing, but a year from now nobody can really tell whether they behave exactly the same, and whether a bug found in one implementation should also be fixed in the other one, etc... . https://reviews.llvm.org/D45977 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.
clayborg added inline comments. Comment at: source/Utility/FileSpec.cpp:107 + for (auto i = path.find('/'); i != llvm::StringRef::npos; + i = path.find('/', i + 1)) { +const auto nextChar = safeCharAtIndex(path, i+1); aprantl wrote: > clayborg wrote: > > I am fine switching to using the llvm functions for removing, this is only > > detecting if any normalization needs to happen. If there is an equivalent > > LLVM function that will only run through the string one time to detect all > > needs for normalization (no multiple passes looking for "..", then for "." > > etc like we used to have), I would be happy to use it, but there doesn't > > seem to be. We are trying to avoid having to create a "SmallVectorImpl< > > char >" for all paths if they don't need fixing here. This function is just > > iterating through an llvm::StringRef just to see if normalization needs to > > happen. If it does, then we make a SmallVectorImpl< char > and we fix the > > path. We can easily use llvm functions for the fixing part. > I see, you want to avoid copying the string when it isn't necessary to do so. > IMHO the best way to do this is to add a `bool hasDots(const Twine &)` > function to llvm::sys::path and add a `bool remove_dot_dot` parameter to > `removeDots()`. Since you have already written the unittests, adding the > function to LLVM shouldn't be any extra work (I'll help reviewing), and we'll > have 100 LoC less to maintain. > > This way we can avoid having two functions that perform path normalization > that superficially look like they might do the same thing, but a year from > now nobody can really tell whether they behave exactly the same, and whether > a bug found in one implementation should also be fixed in the other one, > etc... . I want to know if there are redundant slashes as well. I want something like "bool llvm::sys::has(const Twine &, bool dots, bool dot_dots, bool redundant_slashes)". But I don't see that getting accepted? I just want a "bool llvm:sys::path::contains_redundant_stuff(const Twine &)". Not sure on the name though. needs_normalization? can_be_shortened? Everything in LLVM right now assumes that dots, dot dots, slashes and realpath stuff happen in the same function. Not sure how to break that up without ruining the performance of the one and only loop that should be happening. I like the current approach since it doesn't require chopping up the string into an array and iterating through the array. https://reviews.llvm.org/D45977 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.
clayborg added inline comments. Comment at: source/Utility/FileSpec.cpp:107 + for (auto i = path.find('/'); i != llvm::StringRef::npos; + i = path.find('/', i + 1)) { +const auto nextChar = safeCharAtIndex(path, i+1); clayborg wrote: > aprantl wrote: > > clayborg wrote: > > > I am fine switching to using the llvm functions for removing, this is > > > only detecting if any normalization needs to happen. If there is an > > > equivalent LLVM function that will only run through the string one time > > > to detect all needs for normalization (no multiple passes looking for > > > "..", then for "." etc like we used to have), I would be happy to use it, > > > but there doesn't seem to be. We are trying to avoid having to create a > > > "SmallVectorImpl< char >" for all paths if they don't need fixing here. > > > This function is just iterating through an llvm::StringRef just to see if > > > normalization needs to happen. If it does, then we make a > > > SmallVectorImpl< char > and we fix the path. We can easily use llvm > > > functions for the fixing part. > > I see, you want to avoid copying the string when it isn't necessary to do > > so. IMHO the best way to do this is to add a `bool hasDots(const Twine &)` > > function to llvm::sys::path and add a `bool remove_dot_dot` parameter to > > `removeDots()`. Since you have already written the unittests, adding the > > function to LLVM shouldn't be any extra work (I'll help reviewing), and > > we'll have 100 LoC less to maintain. > > > > This way we can avoid having two functions that perform path normalization > > that superficially look like they might do the same thing, but a year from > > now nobody can really tell whether they behave exactly the same, and > > whether a bug found in one implementation should also be fixed in the other > > one, etc... . > I want to know if there are redundant slashes as well. I want something like > "bool llvm::sys::has(const Twine &, bool dots, bool dot_dots, bool > redundant_slashes)". But I don't see that getting accepted? I just want a > "bool llvm:sys::path::contains_redundant_stuff(const Twine &)". Not sure on > the name though. needs_normalization? can_be_shortened? Everything in LLVM > right now assumes that dots, dot dots, slashes and realpath stuff happen in > the same function. Not sure how to break that up without ruining the > performance of the one and only loop that should be happening. I like the > current approach since it doesn't require chopping up the string into an > array and iterating through the array. Also not sure if the LLVM stuff will try to substitute in the current working directory for "."? https://reviews.llvm.org/D45977 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.
aprantl added inline comments. Comment at: source/Utility/FileSpec.cpp:107 + for (auto i = path.find('/'); i != llvm::StringRef::npos; + i = path.find('/', i + 1)) { +const auto nextChar = safeCharAtIndex(path, i+1); clayborg wrote: > clayborg wrote: > > aprantl wrote: > > > clayborg wrote: > > > > I am fine switching to using the llvm functions for removing, this is > > > > only detecting if any normalization needs to happen. If there is an > > > > equivalent LLVM function that will only run through the string one time > > > > to detect all needs for normalization (no multiple passes looking for > > > > "..", then for "." etc like we used to have), I would be happy to use > > > > it, but there doesn't seem to be. We are trying to avoid having to > > > > create a "SmallVectorImpl< char >" for all paths if they don't need > > > > fixing here. This function is just iterating through an llvm::StringRef > > > > just to see if normalization needs to happen. If it does, then we make > > > > a SmallVectorImpl< char > and we fix the path. We can easily use llvm > > > > functions for the fixing part. > > > I see, you want to avoid copying the string when it isn't necessary to do > > > so. IMHO the best way to do this is to add a `bool hasDots(const Twine > > > &)` function to llvm::sys::path and add a `bool remove_dot_dot` parameter > > > to `removeDots()`. Since you have already written the unittests, adding > > > the function to LLVM shouldn't be any extra work (I'll help reviewing), > > > and we'll have 100 LoC less to maintain. > > > > > > This way we can avoid having two functions that perform path > > > normalization that superficially look like they might do the same thing, > > > but a year from now nobody can really tell whether they behave exactly > > > the same, and whether a bug found in one implementation should also be > > > fixed in the other one, etc... . > > I want to know if there are redundant slashes as well. I want something > > like "bool llvm::sys::has(const Twine &, bool dots, bool dot_dots, bool > > redundant_slashes)". But I don't see that getting accepted? I just want a > > "bool llvm:sys::path::contains_redundant_stuff(const Twine &)". Not sure > > on the name though. needs_normalization? can_be_shortened? Everything in > > LLVM right now assumes that dots, dot dots, slashes and realpath stuff > > happen in the same function. Not sure how to break that up without ruining > > the performance of the one and only loop that should be happening. I like > > the current approach since it doesn't require chopping up the string into > > an array and iterating through the array. > Also not sure if the LLVM stuff will try to substitute in the current working > directory for "."? I just read the source code of remove_dots() (http://llvm.org/doxygen/Path_8cpp_source.html#l00699) and if I'm not mistaken, it actually also removes double-separators as a side-effect, since it iterates over the path components of the string as it constructs the copy. It also seems to avoid the copy operation if it isn't necessary. Could you take another look? Perhaps I'm missing something, (or perhaps we can just turn this into a small addition to remove_dots). > Everything in LLVM right now assumes that dots, dot dots, slashes and > realpath stuff happen in the same function. I'm not sure I understand. https://reviews.llvm.org/D45977 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.
aprantl added a comment. > Also not sure if the LLVM stuff will try to substitute in the current working > directory for "."? No it won't, remove_dots() has no side effects. There's a separate make_absolute() function. That one will cause another copy. Personally, if I have a choice between a redundant string copy and maintaining more code, I'd pick the string copy unless we know that it really matters. https://reviews.llvm.org/D45977 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.
aprantl added a comment. I'll take that back: make_absolute only copies when the path isn't already absolute. https://reviews.llvm.org/D45977 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits