Skip to content

[lldb] Fix a off-by-one error in ParseSupportFilesFromPrologue #71984

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

Merged

Conversation

JDevlieghere
Copy link
Member

@JDevlieghere JDevlieghere commented 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.

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.
@llvmbot
Copy link
Member

llvmbot commented Nov 10, 2023

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

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. One that PR is merged, this will be covered by all the DWARF v5 tests.


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

1 Files Affected:

  • (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (+16-9)
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index aabd83a194932ff..67ea17b2e1fb1bd 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -210,17 +210,24 @@ ParseSupportFilesFromPrologue(const lldb::ModuleSP &module,
                               FileSpec::Style style,
                               llvm::StringRef compile_dir = {}) {
   FileSpecList support_files;
-  size_t first_file = 0;
-  if (prologue.getVersion() <= 4) {
-    // File index 0 is not valid before DWARF v5. Add a dummy entry to ensure
-    // support file list indices match those we get from the debug info and line
-    // tables.
+
+  // Handle the case where there are no files first to avoid having to special
+  // case this later.
+  if (prologue.FileNames.empty())
+    return support_files;
+
+  // Before DWARF v5, the line table indexes were one based.
+  const bool is_one_based = prologue.getVersion() < 5;
+  const size_t file_names = prologue.FileNames.size();
+  const size_t first_file_idx = is_one_based ? 1 : 0;
+  const size_t last_file_idx = is_one_based ? file_names : file_names - 1;
+
+  // Add a dummy entry to ensure the support file list indices match those we
+  // get from the debug info and line tables.
+  if (is_one_based)
     support_files.Append(FileSpec());
-    first_file = 1;
-  }
 
-  const size_t number_of_files = prologue.FileNames.size();
-  for (size_t idx = first_file; idx <= number_of_files; ++idx) {
+  for (size_t idx = first_file_idx; idx <= last_file_idx; ++idx) {
     std::string remapped_file;
     if (auto file_path = GetFileByIndex(prologue, idx, compile_dir, style)) {
       if (auto remapped = module->RemapSourceFile(llvm::StringRef(*file_path)))

Copy link
Contributor

@felipepiovezan felipepiovezan left a comment

Choose a reason for hiding this comment

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

LGTM! really like the _idx suffix of the new induction variable and its bounds

@JDevlieghere JDevlieghere merged commit fa7e07e into llvm:main Nov 10, 2023
@JDevlieghere JDevlieghere deleted the OffByOneInParseSupportFilesFromPrologue branch November 10, 2023 22:03
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).
Copy link
Collaborator

@clayborg clayborg left a comment

Choose a reason for hiding this comment

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

LGTM

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).
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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants