[lldb] Read Checksum from DWARF line tables #71458
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Depends on #71457. |
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesRead the MD5 checksum from DWARF line tables and store it in the Full diff: https://github.com/llvm/llvm-project/pull/71458.diff 9 Files Affected:
diff --git a/lldb/include/lldb/Utility/Checksum.h b/lldb/include/lldb/Utility/Checksum.h
new file mode 100644
index 000000000000000..90a579b247636ac
--- /dev/null
+++ b/lldb/include/lldb/Utility/Checksum.h
@@ -0,0 +1,36 @@
+//===-- Checksum.h ----------------------------------------------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_UTILITY_CHECKSUM_H
+#define LLDB_UTILITY_CHECKSUM_H
+
+#include "llvm/Support/MD5.h"
+
+namespace lldb_private {
+class Checksum {
+public:
+ static llvm::MD5::MD5Result sentinel;
+
+ Checksum(llvm::MD5::MD5Result md5 = sentinel);
+ Checksum(const Checksum &checksum);
+ Checksum &operator=(const Checksum &checksum);
+
+ explicit operator bool() const;
+ bool operator==(const Checksum &checksum) const;
+ bool operator!=(const Checksum &checksum) const;
+
+ std::string digest() const;
+
+private:
+ void SetMD5(llvm::MD5::MD5Result);
+
+ llvm::MD5::MD5Result m_checksum;
+};
+} // namespace lldb_private
+
+#endif // LLDB_UTILITY_CHECKSUM_H
diff --git a/lldb/include/lldb/Utility/FileSpec.h b/lldb/include/lldb/Utility/FileSpec.h
index f06a8543a056e87..29506b01c56d40b 100644
--- a/lldb/include/lldb/Utility/FileSpec.h
+++ b/lldb/include/lldb/Utility/FileSpec.h
@@ -13,15 +13,18 @@
#include <optional>
#include <string>
+#include "lldb/Utility/Checksum.h"
#include "lldb/Utility/ConstString.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/MD5.h"
#include "llvm/Support/Path.h"
#include <cstddef>
#include <cstdint>
+#include <optional>
namespace lldb_private {
class Stream;
@@ -72,7 +75,8 @@ class FileSpec {
/// The style of the path
///
/// \see FileSpec::SetFile (const char *path)
- explicit FileSpec(llvm::StringRef path, Style style = Style::native);
+ explicit FileSpec(llvm::StringRef path, Style style = Style::native,
+ const Checksum &checksum = {});
explicit FileSpec(llvm::StringRef path, const llvm::Triple &triple);
@@ -362,7 +366,11 @@ class FileSpec {
///
/// \param[in] style
/// The style for the given path.
- void SetFile(llvm::StringRef path, Style style);
+ ///
+ /// \param[in] checksum
+ /// The checksum for the given path.
+ void SetFile(llvm::StringRef path, Style style,
+ const Checksum &checksum = {});
/// Change the file specified with a new path.
///
@@ -420,6 +428,9 @@ class FileSpec {
/// The lifetime of the StringRefs is tied to the lifetime of the FileSpec.
std::vector<llvm::StringRef> GetComponents() const;
+ /// Return the checksum for this FileSpec or all zeros if there is none.
+ const Checksum &GetChecksum() const { return m_checksum; };
+
protected:
// Convenience method for setting the file without changing the style.
void SetFile(llvm::StringRef path);
@@ -427,6 +438,7 @@ class FileSpec {
/// Called anytime m_directory or m_filename is changed to clear any cached
/// state in this object.
void PathWasModified() {
+ m_checksum = Checksum();
m_is_resolved = false;
m_absolute = Absolute::Calculate;
}
@@ -443,6 +455,9 @@ class FileSpec {
/// The unique'd filename path.
ConstString m_filename;
+ /// The optional MD5 checksum of the file.
+ Checksum m_checksum;
+
/// True if this path has been resolved.
mutable bool m_is_resolved = false;
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index aabd83a194932ff..79d44bce3d663b6 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -229,8 +229,15 @@ ParseSupportFilesFromPrologue(const lldb::ModuleSP &module,
remapped_file = std::move(*file_path);
}
+ Checksum checksum;
+ if (prologue.ContentTypes.HasMD5) {
+ const llvm::DWARFDebugLine::FileNameEntry &file_name_entry =
+ prologue.getFileNameEntry(idx);
+ checksum = file_name_entry.Checksum;
+ }
+
// Unconditionally add an entry, so the indices match up.
- support_files.EmplaceBack(remapped_file, style);
+ support_files.EmplaceBack(remapped_file, style, checksum);
}
return support_files;
diff --git a/lldb/source/Utility/CMakeLists.txt b/lldb/source/Utility/CMakeLists.txt
index 16afab1113a970c..a3b0a405b4133f6 100644
--- a/lldb/source/Utility/CMakeLists.txt
+++ b/lldb/source/Utility/CMakeLists.txt
@@ -29,6 +29,7 @@ add_lldb_library(lldbUtility NO_INTERNAL_DEPENDENCIES
Args.cpp
Baton.cpp
Broadcaster.cpp
+ Checksum.cpp
CompletionRequest.cpp
Connection.cpp
ConstString.cpp
diff --git a/lldb/source/Utility/Checksum.cpp b/lldb/source/Utility/Checksum.cpp
new file mode 100644
index 000000000000000..70167e497a526c4
--- /dev/null
+++ b/lldb/source/Utility/Checksum.cpp
@@ -0,0 +1,46 @@
+//===-- Checksum.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
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Utility/Checksum.h"
+#include "llvm/ADT/SmallString.h"
+
+using namespace lldb_private;
+
+Checksum::Checksum(llvm::MD5::MD5Result md5) { SetMD5(md5); }
+
+Checksum::Checksum(const Checksum &checksum) { SetMD5(checksum.m_checksum); }
+
+Checksum &Checksum::operator=(const Checksum &checksum) {
+ SetMD5(checksum.m_checksum);
+ return *this;
+}
+
+void Checksum::SetMD5(llvm::MD5::MD5Result md5) {
+ std::uninitialized_copy_n(md5.begin(), 16, m_checksum.begin());
+}
+
+Checksum::operator bool() const {
+ return !std::equal(m_checksum.begin(), m_checksum.end(), sentinel.begin());
+}
+
+bool Checksum::operator==(const Checksum &checksum) const {
+ return std::equal(m_checksum.begin(), m_checksum.end(),
+ checksum.m_checksum.begin());
+}
+
+bool Checksum::operator!=(const Checksum &checksum) const {
+ return !std::equal(m_checksum.begin(), m_checksum.end(),
+ checksum.m_checksum.begin());
+}
+
+std::string Checksum::digest() const {
+ return std::string(m_checksum.digest().str());
+}
+
+llvm::MD5::MD5Result Checksum::sentinel = {0, 0, 0, 0, 0, 0, 0, 0,
+ 0, 0, 0, 0, 0, 0, 0, 0};
diff --git a/lldb/source/Utility/FileSpec.cpp b/lldb/source/Utility/FileSpec.cpp
index eb34ef97cea0b2f..4bbfbb7c1fab5e6 100644
--- a/lldb/source/Utility/FileSpec.cpp
+++ b/lldb/source/Utility/FileSpec.cpp
@@ -68,8 +68,9 @@ void Denormalize(llvm::SmallVectorImpl<char> &path, FileSpec::Style style) {
FileSpec::FileSpec() : m_style(GetNativeStyle()) {}
// Default constructor that can take an optional full path to a file on disk.
-FileSpec::FileSpec(llvm::StringRef path, Style style) : m_style(style) {
- SetFile(path, style);
+FileSpec::FileSpec(llvm::StringRef path, Style style, const Checksum &checksum)
+ : m_checksum(checksum), m_style(style) {
+ SetFile(path, style, checksum);
}
FileSpec::FileSpec(llvm::StringRef path, const llvm::Triple &triple)
@@ -171,9 +172,11 @@ void FileSpec::SetFile(llvm::StringRef pathname) { SetFile(pathname, m_style); }
// Update the contents of this object with a new path. The path will be split
// up into a directory and filename and stored as uniqued string values for
// quick comparison and efficient memory usage.
-void FileSpec::SetFile(llvm::StringRef pathname, Style style) {
+void FileSpec::SetFile(llvm::StringRef pathname, Style style,
+ const Checksum &checksum) {
Clear();
m_style = (style == Style::native) ? GetNativeStyle() : style;
+ m_checksum = checksum;
if (pathname.empty())
return;
diff --git a/lldb/unittests/Utility/CMakeLists.txt b/lldb/unittests/Utility/CMakeLists.txt
index 5c7003a156813dc..097dae860b15911 100644
--- a/lldb/unittests/Utility/CMakeLists.txt
+++ b/lldb/unittests/Utility/CMakeLists.txt
@@ -4,6 +4,7 @@ add_lldb_unittest(UtilityTests
OptionsWithRawTest.cpp
ArchSpecTest.cpp
BroadcasterTest.cpp
+ ChecksumTest.cpp
ConstStringTest.cpp
CompletionRequestTest.cpp
DataBufferTest.cpp
diff --git a/lldb/unittests/Utility/ChecksumTest.cpp b/lldb/unittests/Utility/ChecksumTest.cpp
new file mode 100644
index 000000000000000..7537d30b5ff5b84
--- /dev/null
+++ b/lldb/unittests/Utility/ChecksumTest.cpp
@@ -0,0 +1,57 @@
+//===-- ChecksumTest.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
+//
+//===----------------------------------------------------------------------===//
+
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/Checksum.h"
+
+using namespace lldb_private;
+
+static llvm::MD5::MD5Result hash1 = {0, 1, 2, 3, 4, 5, 6, 7,
+ 8, 9, 10, 11, 12, 13, 14, 15};
+
+static llvm::MD5::MD5Result hash2 = {0, 1, 2, 3, 4, 5, 6, 7,
+ 8, 9, 10, 11, 12, 13, 14, 15};
+
+static llvm::MD5::MD5Result hash3 = {8, 9, 10, 11, 12, 13, 14, 15,
+ 0, 1, 2, 3, 4, 5, 6, 7};
+
+TEST(ChecksumTest, TestConstructor) {
+ Checksum checksum1;
+ EXPECT_FALSE(static_cast<bool>(checksum1));
+ EXPECT_EQ(checksum1, Checksum());
+
+ Checksum checksum2 = Checksum(hash1);
+ EXPECT_EQ(checksum2, Checksum(hash1));
+
+ Checksum checksum3(checksum2);
+ EXPECT_EQ(checksum3, Checksum(hash1));
+}
+
+TEST(ChecksumTest, TestCopyConstructor) {
+ Checksum checksum1;
+ EXPECT_FALSE(static_cast<bool>(checksum1));
+ EXPECT_EQ(checksum1, Checksum());
+
+ Checksum checksum2 = checksum1;
+ EXPECT_EQ(checksum2, checksum1);
+
+ Checksum checksum3(checksum1);
+ EXPECT_EQ(checksum3, checksum1);
+}
+
+TEST(ChecksumTest, TestMD5) {
+ Checksum checksum1(hash1);
+ EXPECT_TRUE(static_cast<bool>(checksum1));
+
+ // Make sure two checksums with the same underlying hashes are the same.
+ EXPECT_EQ(Checksum(hash1), Checksum(hash2));
+
+ // Make sure two checksums with different underlying hashes are different.
+ EXPECT_NE(Checksum(hash1), Checksum(hash3));
+}
diff --git a/lldb/unittests/Utility/FileSpecTest.cpp b/lldb/unittests/Utility/FileSpecTest.cpp
index 2a62f6b1e76120f..ebe7bde7d9c2169 100644
--- a/lldb/unittests/Utility/FileSpecTest.cpp
+++ b/lldb/unittests/Utility/FileSpecTest.cpp
@@ -28,8 +28,8 @@ TEST(FileSpecTest, FileAndDirectoryComponents) {
FileSpec fs_windows("F:\\bar", FileSpec::Style::windows);
EXPECT_STREQ("F:\\bar", fs_windows.GetPath().c_str());
- // EXPECT_STREQ("F:\\", fs_windows.GetDirectory().GetPath().c_str()); // It returns
- // "F:/"
+ // EXPECT_STREQ("F:\\", fs_windows.GetDirectory().GetPath().c_str()); // It
+ // returns "F:/"
EXPECT_STREQ("bar", fs_windows.GetFilename().GetCString());
FileSpec fs_posix_root("/", FileSpec::Style::posix);
@@ -297,45 +297,18 @@ TEST(FileSpecTest, FormatFileSpec) {
TEST(FileSpecTest, IsRelative) {
llvm::StringRef not_relative[] = {
- "/",
- "/a",
- "/a/",
- "/a/b",
- "/a/b/",
- "//",
- "//a/",
- "//a/b",
- "//a/b/",
- "~",
- "~/",
- "~/a",
- "~/a/",
- "~/a/b",
- "~/a/b/",
- "/foo/.",
- "/foo/..",
- "/foo/../",
- "/foo/../.",
+ "/", "/a", "/a/", "/a/b", "/a/b/", "//", "//a/",
+ "//a/b", "//a/b/", "~", "~/", "~/a", "~/a/", "~/a/b",
+ "~/a/b/", "/foo/.", "/foo/..", "/foo/../", "/foo/../.",
};
- for (const auto &path: not_relative) {
+ for (const auto &path : not_relative) {
SCOPED_TRACE(path);
EXPECT_FALSE(PosixSpec(path).IsRelative());
}
llvm::StringRef is_relative[] = {
- ".",
- "./",
- ".///",
- "a",
- "./a",
- "./a/",
- "./a/",
- "./a/b",
- "./a/b/",
- "../foo",
- "foo/bar.c",
- "./foo/bar.c"
- };
- for (const auto &path: is_relative) {
+ ".", "./", ".///", "a", "./a", "./a/",
+ "./a/", "./a/b", "./a/b/", "../foo", "foo/bar.c", "./foo/bar.c"};
+ for (const auto &path : is_relative) {
SCOPED_TRACE(path);
EXPECT_TRUE(PosixSpec(path).IsRelative());
}
@@ -421,7 +394,6 @@ TEST(FileSpecTest, Match) {
EXPECT_TRUE(Match("", "/foo/bar"));
EXPECT_TRUE(Match("", ""));
-
}
TEST(FileSpecTest, TestAbsoluteCaching) {
@@ -534,3 +506,15 @@ TEST(FileSpecTest, TestGetComponents) {
EXPECT_EQ(file_spec.GetComponents(), pair.second);
}
}
+
+TEST(FileSpecTest, TestChecksum) {
+ Checksum checksum({0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15});
+ FileSpec file_spec("/foo/bar", FileSpec::Style::posix, checksum);
+ EXPECT_TRUE(static_cast<bool>(file_spec.GetChecksum()));
+ EXPECT_EQ(file_spec.GetChecksum(), checksum);
+
+ FileSpec copy = file_spec;
+
+ EXPECT_TRUE(static_cast<bool>(copy.GetChecksum()));
+ EXPECT_EQ(file_spec.GetChecksum(), copy.GetChecksum());
+}
|
bulbazord
approved these changes
Nov 7, 2023
medismailben
approved these changes
Nov 7, 2023
clayborg
requested changes
Nov 7, 2023
There was a problem hiding this comment.
I would love to see if we can do this without increasing the size of FileSpec objects. Can we have the DWARF line tables store a FileSpec + checksum and do its warning without making every file object contain a checksum object that most people will never use?
lldb/include/lldb/Utility/FileSpec.h
Outdated
Comment on lines
458
to
461
| /// The optional MD5 checksum of the file. | ||
| Checksum m_checksum; | ||
|
|
There was a problem hiding this comment.
We are increasing the size of all FileSpec objects by 16 bytes here. Is. it possible to do these checks another way? The FileSpec is already 24 bytes, now it will be 40.
There was a problem hiding this comment.
To close the loop on this for others, Greg measured the impact of the size increase in #71457 (thank you!) and we think the size increase is acceptable in the grand scheme of things. As I mentioned in the commit message, we can always revisit this in the future.
Read the MD5 checksum from DWARF line tables and store it in the corresponding support files.
JDevlieghere
force-pushed
the
ReadChecksumInSymbolFileDWARF
branch
from
c6bc958 to
5673248
Compare
|
Hi @JDevlieghere, this change seems to be causing 59 test failures in the cross-project-test suite. Can you take a look? https://lab.llvm.org/buildbot/#/builders/217/builds/30979 |
I'll revert the change, but I doubt this is the cause as this patch should just be storing a member. However there's nothing in the logs to exonerate me so let's see if the revert changes anything. |
JDevlieghere
added a commit
that referenced
this pull request
Nov 9, 2023
This reverts commit 5da98de.
JDevlieghere
added a commit
that referenced
this pull request
Nov 9, 2023
Reverts #71458 as it might have caused cross-project-test failures.
|
Reverted in #71864. |
I did try building the commit immediately before yours, and then yours and that is how I determined it to be caused by your commit. |
Thanks, they seem to be passing now: |
|
It was also passing before with the change, so the issue isn't fully deterministic: https://lab.llvm.org/buildbot/#/builders/217/builds/30986 |
That was me forcing the build of the commit immediately preceding yours to verify it was passing:
|
|
For completeness, here is where I forced a build of the commit immediately preceding yours: That run completed successfully. Then I forced a run with only your changes in https://lab.llvm.org/buildbot/#/builders/217/builds/30987 That run had 59 failures. |
|
That would've been helpful information to include in your original message and saved me the time of having to go through the logs. |
Sorry about that, I'll be sure to do so next time. |
JDevlieghere
added a commit
to JDevlieghere/llvm-project
that referenced
this pull request
Nov 10, 2023
This fixes a subtle and previously harmless off-by-one bug in ParseSupportFilesFromPrologue. The function accounts for the start index being one-based for DWARF v4 and earlier and zero-based for DWARF v5 and later. However, the same care wasn't taken for the end index. This bug existed unnoticed because GetFileByIndex gracefully handles an invalid index. However, the bug manifested itself after llvm#71458, which added a call to getFileNameEntry which requires the index to be valid. No test as the bug cannot be observed without the changes from llvm#71458. One that PR is merged, this will be covered by all the DWARF v5 tests.
JDevlieghere
added a commit
that referenced
this pull request
Nov 10, 2023
This fixes a subtle and previously harmless off-by-one bug in ParseSupportFilesFromPrologue. The function accounts for the start index being one-based for DWARF v4 and earlier and zero-based for DWARF v5 and later. However, the same care wasn't taken for the end index. This bug existed unnoticed because GetFileByIndex gracefully handles an invalid index. However, the bug manifested itself after #71458, which added a call to getFileNameEntry which requires the index to be valid. No test as the bug cannot be observed without the changes from #71458. Once that PR is merged, this will be covered by all the DWARF v5 tests.
JDevlieghere
added a commit
that referenced
this pull request
Nov 10, 2023
Read the MD5 checksum from DWARF line tables and store it in the corresponding support files. This is a re-land after fixing an off-by-one error in LLDB's ParseSupportFilesFromPrologue (fixed in #71984).
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
Read the MD5 checksum from DWARF line tables and store it in the corresponding support files.
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
Reverts llvm#71458 as it might have caused cross-project-test failures.
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
…71984) This fixes a subtle and previously harmless off-by-one bug in ParseSupportFilesFromPrologue. The function accounts for the start index being one-based for DWARF v4 and earlier and zero-based for DWARF v5 and later. However, the same care wasn't taken for the end index. This bug existed unnoticed because GetFileByIndex gracefully handles an invalid index. However, the bug manifested itself after llvm#71458, which added a call to getFileNameEntry which requires the index to be valid. No test as the bug cannot be observed without the changes from llvm#71458. Once that PR is merged, this will be covered by all the DWARF v5 tests.
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
Read the MD5 checksum from DWARF line tables and store it in the corresponding support files. This is a re-land after fixing an off-by-one error in LLDB's ParseSupportFilesFromPrologue (fixed in llvm#71984).
Guzhu-AMD
pushed a commit
to GPUOpen-Drivers/llvm-project
that referenced
this pull request
Nov 23, 2023
Local branch amd-gfx b47928a Merged main:237adfca4ef8 into amd-gfx:5c6a35ee21ea Remote branch main 64f62de [lldb] Read Checksum from DWARF line tables (llvm#71458)
adrian-prantl
pushed a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Jan 4, 2024
…71984) This fixes a subtle and previously harmless off-by-one bug in ParseSupportFilesFromPrologue. The function accounts for the start index being one-based for DWARF v4 and earlier and zero-based for DWARF v5 and later. However, the same care wasn't taken for the end index. This bug existed unnoticed because GetFileByIndex gracefully handles an invalid index. However, the bug manifested itself after llvm#71458, which added a call to getFileNameEntry which requires the index to be valid. No test as the bug cannot be observed without the changes from llvm#71458. Once that PR is merged, this will be covered by all the DWARF v5 tests. (cherry picked from commit fa7e07e)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Read the MD5 checksum from DWARF line tables and store it in the
corresponding support files.