Fix crash due to un-checked error in LVReaderHandler::handleArchive method #118951

aurelien35 · 2024-12-06T10:28:47Z

Fix crash due to un-checked error in LVReaderHandler::handleArchive method.

Correction thanks to @CarlosAlbertoEnciso suggestion

@CarlosAlbertoEnciso

Fix crash due to un-checked error in LVReaderHandler::handleArchive method. Correction thanks to @CarlosAlbertoEnciso suggestion

github-actions · 2024-12-06T10:29:08Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-12-06T10:29:45Z

@llvm/pr-subscribers-llvm-binary-utilities

@llvm/pr-subscribers-debuginfo

Author: None (aurelien35)

Changes

Fix crash due to un-checked error in LVReaderHandler::handleArchive method.

Correction thanks to @CarlosAlbertoEnciso suggestion

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

1 Files Affected:

(modified) llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp (+3)

diff --git a/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp b/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp
index 71750f3d114c11..7d64100c60c21a 100644
--- a/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp
+++ b/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp
@@ -88,6 +88,9 @@ Error LVReaderHandler::handleArchive(LVReaders &Readers, StringRef Filename,
                                Filename.str().c_str());
   }
 
+  if (Err)
+    return createStringError(errorToErrorCode(std::move(Err)), "%s",
+                               Filename.str().c_str());
   return Error::success();
 }

aurelien35 · 2024-12-06T10:31:10Z

Suggested fix for this issue: #118753

CarlosAlbertoEnciso · 2024-12-06T15:35:26Z

@aurelien35 Can you add a test case that loads a library? Thanks

aurelien35 · 2024-12-09T07:43:46Z

Yes, I think this is a good idea, but I never worked on the LLVM tests suite.
I've looked a little to the "tests" subdirs but I'm not sure how to write a new test.
Is there any resources available on that topic?

CarlosAlbertoEnciso · 2024-12-09T17:32:29Z

This is the directory for the llvm-debuginfo-analyzer tests. They are divided into DWARF, COFF and WebAssembly.

You can check the tests for llvm-dwarfdump archive. There is already an input file.

The test file can be in the DWARF directory.
The command line for the test file would include some lines like:

; RUN: llvm-debuginfo-analyzer --attribute=format \
; RUN:                         --print=scopes \
; RUN:                         %S/../../dsymutil/Inputs/libfat-test.a 2>&1 | \
; RUN: FileCheck --strict-whitespace %s

; CHECK:      Logical View:
; CHECK-NEXT: [000]           {File} 'libfat-test.a' -> Mach-O 64-bit x86-64
; CHECK-EMPTY:
; CHECK-NEXT: [001]             {CompileUnit} 'fat-test.c'
more lines ...

Additional information: filecheck, llvm testing.

Any question, please do let me know. Happy to help.

aurelien35 · 2024-12-10T20:35:55Z

I've extended the llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp unit test with a unit test to load a library file.

Without the merge request, it fails.
With it, it succeed.

I've made the .lib file using the "lib.exe" tool from msvc on the "llvm/unittests/DebugInfo/LogicalView/Inputs/test-codeview-msvc.o" file.

I'm investigating further for other bugs concerning MSVC static libraries debug info loading.

Do you know who can I contact about the MSVC CodeView information parsing specific topic?

CarlosAlbertoEnciso · 2024-12-10T23:49:10Z

This was the starting documentation I used for the llvm-debuginfo-analyzer CodeView support.

A presentation done by Reid Kleckner
https://llvm.org/devmtg/2016-11/Slides/Kleckner-CodeViewInLLVM.pdf
@rnk

rnk · 2024-12-11T00:13:13Z

Thanks for the fix!

@tstellar @tru : note the use of the binary file for unittest purposes here. I don't see a reasonable way to avoid this file and retain this test with today's test infra system.

github-actions · 2024-12-11T00:16:52Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff e9866d5d149706eac26f45bf0cab933c51d6d1cd 336844c32c43938866c3f315af745eddb1691292 --extensions cpp -- llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp b/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp
index 7d64100c60..69513f2b98 100644
--- a/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp
+++ b/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp
@@ -90,7 +90,7 @@ Error LVReaderHandler::handleArchive(LVReaders &Readers, StringRef Filename,
 
   if (Err)
     return createStringError(errorToErrorCode(std::move(Err)), "%s",
-                               Filename.str().c_str());
+                             Filename.str().c_str());
   return Error::success();
 }
 
diff --git a/llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp b/llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp
index c05114541d..7639f001e3 100644
--- a/llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp
+++ b/llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp
@@ -32,7 +32,8 @@ namespace {
 const char *CodeViewClang = "test-codeview-clang.o";
 const char *CodeViewMsvc = "test-codeview-msvc.o";
 const char *CodeViewMsvcLib = "test-codeview-msvc.lib";
-const char *CodeViewMsvcLibContentName = "test-codeview-msvc.lib(test-codeview-msvc.o)";
+const char *CodeViewMsvcLibContentName =
+    "test-codeview-msvc.lib(test-codeview-msvc.o)";
 const char *CodeViewPdbMsvc = "test-codeview-pdb-msvc.o";
 
 // Helper function to get the first scope child from the given parent.

CarlosAlbertoEnciso · 2024-12-11T00:24:06Z

Quite interesting your approach to use a unit test to load a MSVC library.

tstellar · 2024-12-11T03:16:31Z

Would it be possible to add a README or something that explains how the file was created?

aurelien35 · 2024-12-11T07:04:07Z

@tstellar : I've added a README.md file which explains how I generated the .lib file

However, I don't know how were generated the other binary files in this directory

CarlosAlbertoEnciso · 2024-12-11T13:31:51Z

@aurelien35 I can check my notes about the steps used to generate the other binary files.

CarlosAlbertoEnciso · 2024-12-12T16:36:03Z

@aurelien35 This is the information extracted from my notes.

Source file: test.cpp

using INTPTR = const int *;
int foo(INTPTR ParamPtr, unsigned ParamUnsigned, bool ParamBool) {
  if (ParamBool) {
    typedef int INTEGER;
    const INTEGER CONSTANT = 7;
    return CONSTANT;
  }
  return ParamUnsigned;
}

Command line used to generate the binary files:

-- Linux

clang -c -w -g -gdwarf-4 -O0 test.cpp -o test-dwarf-clang.o

g++ -c -w -g -O0 test.cpp -o test-dwarf-gcc.o

-- Windows

clang++.exe --target=x86_64-windows -c -w -g -gcodeview test.cpp -o test-codeview-clang.o

cl.exe /nologo /EHsc /Od /Z7 /Fotest-codeview-msvc.o /c test.cpp
cl.exe /nologo /EHsc /Od /Zi /Fotest-codeview-pdb-msvc.o /Fdtest-codeview-pdb-msvc-.pdb /c test.cpp

aurelien35 · 2024-12-12T21:05:08Z

@CarlosAlbertoEnciso I've updated the README.md file with your notes

CarlosAlbertoEnciso · 2024-12-13T12:48:50Z

@pogo59 Do you have any comments on this patch? Thanks

CarlosAlbertoEnciso · 2024-12-13T13:21:14Z

llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp

@@ -193,6 +196,72 @@ void checkElementPropertiesMsvcCodeview(LVReader *Reader) {
  EXPECT_EQ(Lines->size(), 0x0eu);
 }

+// Check the logical elements basic properties (MSVC library - Codeview).
+void checkElementPropertiesMsvcLibraryCodeview(LVReader *Reader) {
+  LVScopeRoot *Root = Reader->getScopesRoot();


Check that Root is non-null
ASSERT_NE(Root, nullptr);

I will add this check in a separated patch.

CarlosAlbertoEnciso · 2024-12-13T13:22:31Z

llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp

+  LVScopeRoot *Root = Reader->getScopesRoot();
+  LVScopeCompileUnit *CompileUnit =
+      static_cast<LVScopeCompileUnit *>(getFirstScopeChild(Root));
+  LVScopeFunction *Function =


Same check here for CompileUnit and Function.

I will add this check in a separated patch.

CarlosAlbertoEnciso · 2024-12-13T13:27:06Z

llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp

+  const LVLocations *Ranges = Function->getRanges();
+  ASSERT_NE(Ranges, nullptr);
+  ASSERT_EQ(Ranges->size(), 1u);
+  LVLocations::const_iterator IterLocation = Ranges->begin();


Check the iterators for end
ASSERT_NE(IterLocation, Ranges->end());

I will add this check in a separated patch.

pogo59 · 2024-12-13T13:38:35Z

@pogo59 Do you have any comments on this patch? Thanks

No, it's fine.

CarlosAlbertoEnciso · 2024-12-13T13:57:14Z

@aurelien35 The patch looks good.

aurelien35 · 2024-12-13T16:06:44Z

@CarlosAlbertoEnciso Thanks.

What's next now? Should I do something or just wait for an admin to merge this patch?

CarlosAlbertoEnciso · 2024-12-13T17:07:14Z

@aurelien35 Do you commit access?

aurelien35 · 2024-12-13T17:15:15Z

@CarlosAlbertoEnciso no, I don't have commit access

CarlosAlbertoEnciso · 2024-12-13T19:09:13Z

@aurelien35 Did you added the suggested checks? I can commit for you.

github-actions · 2024-12-17T11:20:07Z

@aurelien35 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

CarlosAlbertoEnciso · 2024-12-17T11:29:36Z

@aurelien35 Committed the patch for you.
You can check the builds here https://lab.llvm.org/buildbot/#/changes/20839
Thanks for fixing the crash.

Update LVReaderHandler.cpp

40c944b

Fix crash due to un-checked error in LVReaderHandler::handleArchive method. Correction thanks to @CarlosAlbertoEnciso suggestion

llvmbot added the debuginfo label Dec 6, 2024

aurelien35 mentioned this pull request Dec 6, 2024

[LogicalView] - Crash when loading a library #118753

Open

CarlosAlbertoEnciso added llvm:binary-utilities tools:llvm-debuginfo-analyzer labels Dec 6, 2024

aurelien35 and others added 5 commits December 9, 2024 21:46

Merge branch 'llvm:main' into patch-1

f48fab6

Add MSVC CodeView informations read from library test case

4f2e094

Merge branch 'llvm:main' into patch-1

c7cf1e1

Merge branch 'llvm:main' into main

8a5973f

Merge branch 'main' of github.com:aurelien35/llvm-project into patch-1

336844c

aurelien35 added 5 commits December 11, 2024 07:56

Create README for LogicalView unit tests inputs generation

3212dae

Update README

a64d550

Use markdown extension for README file

d86c8dd

Delete llvm/unittests/DebugInfo/LogicalView/Inputs/README

81ba620

Merge branch 'llvm:main' into patch-1

7db47c2

aurelien35 added 2 commits December 11, 2024 08:05

Fix CodeViewReaderTest.cpp code formatting

ae340e3

Fix LVReaderHandler.cpp code formatting

f164a86

Your Name and others added 2 commits December 12, 2024 22:01

Update LogicalView unit tests inputs README file

1944610

Merge branch 'main' into patch-1

df64aff

CarlosAlbertoEnciso reviewed Dec 13, 2024

View reviewed changes

CarlosAlbertoEnciso merged commit 37e48e4 into llvm:main Dec 17, 2024
7 checks passed

Fix crash due to un-checked error in LVReaderHandler::handleArchive method #118951

Fix crash due to un-checked error in LVReaderHandler::handleArchive method #118951

Uh oh!

Conversation

aurelien35 commented Dec 6, 2024

Uh oh!

github-actions bot commented Dec 6, 2024

Uh oh!

llvmbot commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aurelien35 commented Dec 6, 2024

Uh oh!

CarlosAlbertoEnciso commented Dec 6, 2024

Uh oh!

aurelien35 commented Dec 9, 2024

Uh oh!

CarlosAlbertoEnciso commented Dec 9, 2024

Uh oh!

aurelien35 commented Dec 10, 2024

Uh oh!

CarlosAlbertoEnciso commented Dec 10, 2024

Uh oh!

rnk commented Dec 11, 2024

Uh oh!

github-actions bot commented Dec 11, 2024

Uh oh!

CarlosAlbertoEnciso commented Dec 11, 2024

Uh oh!

tstellar commented Dec 11, 2024

Uh oh!

aurelien35 commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CarlosAlbertoEnciso commented Dec 11, 2024

Uh oh!

CarlosAlbertoEnciso commented Dec 12, 2024

Uh oh!

aurelien35 commented Dec 12, 2024

Uh oh!

CarlosAlbertoEnciso commented Dec 13, 2024

Uh oh!

CarlosAlbertoEnciso Dec 13, 2024

Choose a reason for hiding this comment

Uh oh!

CarlosAlbertoEnciso Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

CarlosAlbertoEnciso Dec 13, 2024

Choose a reason for hiding this comment

Uh oh!

CarlosAlbertoEnciso Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

CarlosAlbertoEnciso Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarlosAlbertoEnciso Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

pogo59 commented Dec 13, 2024

Uh oh!

CarlosAlbertoEnciso commented Dec 13, 2024

Uh oh!

aurelien35 commented Dec 13, 2024

Uh oh!

CarlosAlbertoEnciso commented Dec 13, 2024

Uh oh!

aurelien35 commented Dec 13, 2024

Uh oh!

CarlosAlbertoEnciso commented Dec 13, 2024

Uh oh!

Uh oh!

github-actions bot commented Dec 17, 2024

Uh oh!

CarlosAlbertoEnciso commented Dec 17, 2024

Uh oh!

Uh oh!

llvmbot commented Dec 6, 2024 •

edited

Loading

aurelien35 commented Dec 11, 2024 •

edited

Loading

CarlosAlbertoEnciso Dec 13, 2024 •

edited

Loading