Skip to content

[lldb] Print a warning on checksum mismatch #71459

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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions lldb/include/lldb/Core/SourceManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
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 ?

#include "llvm/Support/RWMutex.h"

#include <cstddef>
Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand Down
36 changes: 36 additions & 0 deletions lldb/include/lldb/Utility/Checksum.h
Original file line number Diff line number Diff line change
@@ -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
19 changes: 17 additions & 2 deletions lldb/include/lldb/Utility/FileSpec.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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.
///
Expand Down Expand Up @@ -420,13 +428,17 @@ 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);

/// 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;
}
Expand All @@ -443,6 +455,9 @@ class FileSpec {
/// The unique'd filename path.
ConstString m_filename;

/// The optional MD5 checksum of the file.
Checksum m_checksum;

Comment on lines +458 to +460
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.

/// True if this path has been resolved.
mutable bool m_is_resolved = false;

Expand Down
28 changes: 22 additions & 6 deletions lldb/source/Core/SourceManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 "
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()).

"info (%s) and the file on disk (%s).\n",
checksum.digest().c_str(),
last_file_sp->GetChecksum().digest().c_str());
});
}
}
return *delta;
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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());
}
}
9 changes: 8 additions & 1 deletion lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions lldb/source/Utility/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
46 changes: 46 additions & 0 deletions lldb/source/Utility/Checksum.cpp
Original file line number Diff line number Diff line change
@@ -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};
9 changes: 6 additions & 3 deletions lldb/source/Utility/FileSpec.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions lldb/unittests/Utility/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ add_lldb_unittest(UtilityTests
OptionsWithRawTest.cpp
ArchSpecTest.cpp
BroadcasterTest.cpp
ChecksumTest.cpp
ConstStringTest.cpp
CompletionRequestTest.cpp
DataBufferTest.cpp
Expand Down
57 changes: 57 additions & 0 deletions lldb/unittests/Utility/ChecksumTest.cpp
Original file line number Diff line number Diff line change
@@ -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));
}
Loading