Skip to content
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

[lldb] Print a warning on checksum mismatch #71459

Closed
wants to merge 4 commits into from

Conversation

JDevlieghere
Copy link
Member

Print a warning when the debugger detects a mismatch between the MD5
checksum in the DWARF 5 line table and the file on disk.

This commit adds an MD5 checksum (`Checksum`) class to LLDB. Its purpose
is to store the MD5 hash added to the DWARF 5 line table.
Store a Checksum in FileSpec. Its purpose is to store the MD5 hash added
to the DWARF 5 line table.
Read the MD5 checksum from DWARF line tables and store it in the
corresponding support files.
Print a warning when the debugger detects a mismatch between the MD5
checksum in the DWARF 5 line table and the file on disk.
@llvmbot llvmbot added the lldb label Nov 6, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 6, 2023

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

Print a warning when the debugger detects a mismatch between the MD5
checksum in the DWARF 5 line table and the file on disk.


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

11 Files Affected:

  • (modified) lldb/include/lldb/Core/SourceManager.h (+9)
  • (added) lldb/include/lldb/Utility/Checksum.h (+36)
  • (modified) lldb/include/lldb/Utility/FileSpec.h (+17-2)
  • (modified) lldb/source/Core/SourceManager.cpp (+22-6)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (+8-1)
  • (modified) lldb/source/Utility/CMakeLists.txt (+1)
  • (added) lldb/source/Utility/Checksum.cpp (+46)
  • (modified) lldb/source/Utility/FileSpec.cpp (+6-3)
  • (modified) lldb/unittests/Utility/CMakeLists.txt (+1)
  • (added) lldb/unittests/Utility/ChecksumTest.cpp (+57)
  • (modified) lldb/unittests/Utility/FileSpecTest.cpp (+21-37)
diff --git a/lldb/include/lldb/Core/SourceManager.h b/lldb/include/lldb/Core/SourceManager.h
index 5239ac6f4055f5d..0296657451c1b8c 100644
--- a/lldb/include/lldb/Core/SourceManager.h
+++ b/lldb/include/lldb/Core/SourceManager.h
@@ -9,11 +9,13 @@
 #ifndef LLDB_CORE_SOURCEMANAGER_H
 #define LLDB_CORE_SOURCEMANAGER_H
 
+#include "lldb/Utility/Checksum.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-forward.h"
 
 #include "llvm/Support/Chrono.h"
+#include "llvm/Support/MD5.h"
 #include "llvm/Support/RWMutex.h"
 
 #include <cstddef>
@@ -68,6 +70,10 @@ class SourceManager {
 
     llvm::sys::TimePoint<> GetTimestamp() const { return m_mod_time; }
 
+    const Checksum &GetChecksum() const { return m_checksum; }
+
+    llvm::once_flag &GetChecksumOnceFlag() { return m_checksum_once_flag; }
+
   protected:
     /// Set file and update modification time.
     void SetFileSpec(FileSpec file_spec);
@@ -83,6 +89,9 @@ class SourceManager {
     // Keep the modification time that this file data is valid for
     llvm::sys::TimePoint<> m_mod_time;
 
+    Checksum m_checksum;
+    llvm::once_flag m_checksum_once_flag;
+
     // If the target uses path remappings, be sure to clear our notion of a
     // source file if the path modification ID changes
     uint32_t m_source_map_mod_id = 0;
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/Core/SourceManager.cpp b/lldb/source/Core/SourceManager.cpp
index 517a4b0268d2a00..d1946d34dfe34fe 100644
--- a/lldb/source/Core/SourceManager.cpp
+++ b/lldb/source/Core/SourceManager.cpp
@@ -300,6 +300,16 @@ size_t SourceManager::DisplaySourceLinesWithLineNumbersUsingLastFile(
         break;
       }
     }
+
+    Checksum checksum = last_file_sp->GetFileSpec().GetChecksum();
+    if (checksum && checksum != last_file_sp->GetChecksum()) {
+      llvm::call_once(last_file_sp->GetChecksumOnceFlag(), [&]() {
+        s->Printf("warning: source file checksum mismatch between the debug "
+                  "info (%s) and the file on disk (%s).\n",
+                  checksum.digest().c_str(),
+                  last_file_sp->GetChecksum().digest().c_str());
+      });
+    }
   }
   return *delta;
 }
@@ -446,13 +456,13 @@ void SourceManager::FindLinesMatchingRegex(FileSpec &file_spec,
 
 SourceManager::File::File(const FileSpec &file_spec,
                           lldb::DebuggerSP debugger_sp)
-    : m_file_spec_orig(file_spec), m_file_spec(), m_mod_time(),
+    : m_file_spec_orig(file_spec), m_file_spec(), m_mod_time(), m_checksum(),
       m_debugger_wp(debugger_sp), m_target_wp(TargetSP()) {
   CommonInitializer(file_spec, {});
 }
 
 SourceManager::File::File(const FileSpec &file_spec, TargetSP target_sp)
-    : m_file_spec_orig(file_spec), m_file_spec(), m_mod_time(),
+    : m_file_spec_orig(file_spec), m_file_spec(), m_mod_time(), m_checksum(),
       m_debugger_wp(target_sp ? target_sp->GetDebugger().shared_from_this()
                               : DebuggerSP()),
       m_target_wp(target_sp) {
@@ -523,14 +533,17 @@ void SourceManager::File::CommonInitializer(const FileSpec &file_spec,
   }
 
   // If the file exists, read in the data.
-  if (m_mod_time != llvm::sys::TimePoint<>())
+  if (m_mod_time != llvm::sys::TimePoint<>()) {
     m_data_sp = FileSystem::Instance().CreateDataBuffer(m_file_spec);
+    m_checksum = llvm::MD5::hash(m_data_sp->GetData());
+  }
 }
 
 void SourceManager::File::SetFileSpec(FileSpec file_spec) {
   resolve_tilde(file_spec);
   m_file_spec = std::move(file_spec);
   m_mod_time = FileSystem::Instance().GetModificationTime(m_file_spec);
+  m_checksum = file_spec.GetChecksum();
 }
 
 uint32_t SourceManager::File::GetLineOffset(uint32_t line) {
@@ -821,13 +834,16 @@ SourceManager::FileSP SourceManager::SourceFileCache::FindSourceFile(
 }
 
 void SourceManager::SourceFileCache::Dump(Stream &stream) const {
-  stream << "Modification time   Lines    Path\n";
-  stream << "------------------- -------- --------------------------------\n";
+  stream
+      << "Modification time   MD5 Checksum                     Lines    Path\n";
+  stream << "------------------- -------------------------------- -------- "
+            "--------------------------------\n";
   for (auto &entry : m_file_cache) {
     if (!entry.second)
       continue;
     FileSP file = entry.second;
-    stream.Format("{0:%Y-%m-%d %H:%M:%S} {1,8:d} {2}\n", file->GetTimestamp(),
+    stream.Format("{0:%Y-%m-%d %H:%M:%S} {1} {2,8:d} {3}\n",
+                  file->GetTimestamp(), file->GetChecksum().digest(),
                   file->GetNumLines(), entry.first.GetPath());
   }
 }
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());
+}

@JDevlieghere
Copy link
Member Author

Depends on #71458. This is probably incomplete and obviously needs a test, but I wanted to put up the PR to show what the dependent patches are building towards.

Copy link
Member

@medismailben medismailben left a comment

Choose a reason for hiding this comment

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

LGTM with comments.

#include "lldb/Utility/FileSpec.h"
#include "lldb/lldb-defines.h"
#include "lldb/lldb-forward.h"

#include "llvm/Support/Chrono.h"
#include "llvm/Support/MD5.h"
Copy link
Member

Choose a reason for hiding this comment

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

Is that necessary ?

Checksum checksum = last_file_sp->GetFileSpec().GetChecksum();
if (checksum && checksum != last_file_sp->GetChecksum()) {
llvm::call_once(last_file_sp->GetChecksumOnceFlag(), [&]() {
s->Printf("warning: source file checksum mismatch between the debug "
Copy link
Member

Choose a reason for hiding this comment

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

Should we make a DiagnosticReport here ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree. Wherever we hardcode the string "error:" or "warning:" we should probably use a more high-level API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

We could use what I'm working on in my other PR, but we'd have to change this to pass the CommandReturnObject instead of just its Stream instance (via &result..GetOutputStream()).

Comment on lines +458 to +460
/// The optional MD5 checksum of the file.
Checksum m_checksum;

Copy link
Collaborator

Choose a reason for hiding this comment

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

This will increase the byte size of every FileSpec object by 16 bytes which is not good. Can we store the checksum somewhere else? Or make a lldb_private::ChecksumFile which has a lldb_private::FileSpec + Checksum?

(lldb) p sizeof(llvm::MD5::MD5Result)
(unsigned long) 16
(lldb) p sizeof(lldb_private::FileSpec)
(unsigned long) 24

Copy link
Collaborator

Choose a reason for hiding this comment

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

The DWARF 5 line table should have a file list where can we easily compute this and warn if needed. It would be fine to store an extra bit in lldb_private::FileSpec that specifies if the checksum has been verified or not, but we would like to keep lldb_private::FileSpec to be 24 bytes or less.

@JDevlieghere
Copy link
Member Author

Closing in favor of #107968

@JDevlieghere JDevlieghere deleted the PrintChecksumMismatch branch September 10, 2024 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants