[llvm-debuginfo-analyzer] Common handling of unsigned attribute values. #116027
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
- In the DWARF reader, for those attributes that can have an unsigned value, allow for the following cases: * Is an implicit constant * Is an optional value
CarlosAlbertoEnciso
requested review from
OCHyams,
SLTozer,
dwblaikie,
jmorse,
jryans and
pogo59
Member
|
@llvm/pr-subscribers-llvm-binary-utilities @llvm/pr-subscribers-debuginfo Author: Carlos Alberto Enciso (CarlosAlbertoEnciso) Changes
Full diff: https://github.com/llvm/llvm-project/pull/116027.diff 2 Files Affected:
diff --git a/llvm/lib/DebugInfo/LogicalView/Readers/LVDWARFReader.cpp b/llvm/lib/DebugInfo/LogicalView/Readers/LVDWARFReader.cpp
index ce1d5619e1fa80..b9ba3357002cab 100644
--- a/llvm/lib/DebugInfo/LogicalView/Readers/LVDWARFReader.cpp
+++ b/llvm/lib/DebugInfo/LogicalView/Readers/LVDWARFReader.cpp
@@ -254,15 +254,18 @@ void LVDWARFReader::processOneAttribute(const DWARFDie &Die,
// We are processing .debug_info section, implicit_const attribute
// values are not really stored here, but in .debug_abbrev section.
auto GetAsUnsignedConstant = [&]() -> int64_t {
- return AttrSpec.isImplicitConst() ? AttrSpec.getImplicitConstValue()
- : *FormValue.getAsUnsignedConstant();
+ if (AttrSpec.isImplicitConst())
+ return AttrSpec.getImplicitConstValue();
+ if (std::optional<uint64_t> Val = FormValue.getAsUnsignedConstant())
+ return *Val;
+ return 0;
};
auto GetFlag = [](const DWARFFormValue &FormValue) -> bool {
return FormValue.isFormClass(DWARFFormValue::FC_Flag);
};
- auto GetBoundValue = [](const DWARFFormValue &FormValue) -> int64_t {
+ auto GetBoundValue = [&AttrSpec](const DWARFFormValue &FormValue) -> int64_t {
switch (FormValue.getForm()) {
case dwarf::DW_FORM_ref_addr:
case dwarf::DW_FORM_ref1:
@@ -283,6 +286,8 @@ void LVDWARFReader::processOneAttribute(const DWARFDie &Die,
return *FormValue.getAsUnsignedConstant();
case dwarf::DW_FORM_sdata:
return *FormValue.getAsSignedConstant();
+ case dwarf::DW_FORM_implicit_const:
+ return AttrSpec.getImplicitConstValue();
default:
return 0;
}
@@ -295,13 +300,13 @@ void LVDWARFReader::processOneAttribute(const DWARFDie &Die,
switch (AttrSpec.Attr) {
case dwarf::DW_AT_accessibility:
- CurrentElement->setAccessibilityCode(*FormValue.getAsUnsignedConstant());
+ CurrentElement->setAccessibilityCode(GetAsUnsignedConstant());
break;
case dwarf::DW_AT_artificial:
CurrentElement->setIsArtificial();
break;
case dwarf::DW_AT_bit_size:
- CurrentElement->setBitSize(*FormValue.getAsUnsignedConstant());
+ CurrentElement->setBitSize(GetAsUnsignedConstant());
break;
case dwarf::DW_AT_call_file:
CurrentElement->setCallFilenameIndex(IncrementFileIndex
@@ -333,13 +338,12 @@ void LVDWARFReader::processOneAttribute(const DWARFDie &Die,
Stream << hexString(Value, 2);
CurrentElement->setValue(Stream.str());
} else
- CurrentElement->setValue(
- hexString(*FormValue.getAsUnsignedConstant(), 2));
+ CurrentElement->setValue(hexString(GetAsUnsignedConstant(), 2));
} else
CurrentElement->setValue(dwarf::toStringRef(FormValue));
break;
case dwarf::DW_AT_count:
- CurrentElement->setCount(*FormValue.getAsUnsignedConstant());
+ CurrentElement->setCount(GetAsUnsignedConstant());
break;
case dwarf::DW_AT_decl_line:
CurrentElement->setLineNumber(GetAsUnsignedConstant());
@@ -358,10 +362,10 @@ void LVDWARFReader::processOneAttribute(const DWARFDie &Die,
CurrentElement->setIsExternal();
break;
case dwarf::DW_AT_GNU_discriminator:
- CurrentElement->setDiscriminator(*FormValue.getAsUnsignedConstant());
+ CurrentElement->setDiscriminator(GetAsUnsignedConstant());
break;
case dwarf::DW_AT_inline:
- CurrentElement->setInlineCode(*FormValue.getAsUnsignedConstant());
+ CurrentElement->setInlineCode(GetAsUnsignedConstant());
break;
case dwarf::DW_AT_lower_bound:
CurrentElement->setLowerBound(GetBoundValue(FormValue));
@@ -381,7 +385,7 @@ void LVDWARFReader::processOneAttribute(const DWARFDie &Die,
CurrentElement->setUpperBound(GetBoundValue(FormValue));
break;
case dwarf::DW_AT_virtuality:
- CurrentElement->setVirtualityCode(*FormValue.getAsUnsignedConstant());
+ CurrentElement->setVirtualityCode(GetAsUnsignedConstant());
break;
case dwarf::DW_AT_abstract_origin:
diff --git a/llvm/test/tools/llvm-debuginfo-analyzer/DWARF/05-dwarf-incorrect-lexical-scope-variable.test b/llvm/test/tools/llvm-debuginfo-analyzer/DWARF/05-dwarf-incorrect-lexical-scope-variable.test
index e1ac7588f1d8c4..3c3c5dcbda520b 100644
--- a/llvm/test/tools/llvm-debuginfo-analyzer/DWARF/05-dwarf-incorrect-lexical-scope-variable.test
+++ b/llvm/test/tools/llvm-debuginfo-analyzer/DWARF/05-dwarf-incorrect-lexical-scope-variable.test
@@ -43,14 +43,14 @@
; ONE-EMPTY:
; ONE-NEXT: [001] {CompileUnit} 'pr-43860.cpp'
; ONE-NEXT: [002] {Producer} 'clang version 15.0.0 {{.*}}'
-; ONE-NEXT: [002] 2 {Function} extern not_inlined 'InlineFunction' -> 'int'
+; ONE-NEXT: [002] 2 {Function} extern inlined 'InlineFunction' -> 'int'
; ONE-NEXT: [003] {Block}
; ONE-NEXT: [004] 5 {Variable} 'Var_2' -> 'int'
; ONE-NEXT: [003] 2 {Parameter} 'Param' -> 'int'
; ONE-NEXT: [003] 3 {Variable} 'Var_1' -> 'int'
; ONE-NEXT: [002] 11 {Function} extern not_inlined 'test' -> 'int'
; ONE-NEXT: [003] 12 {Variable} 'A' -> 'int'
-; ONE-NEXT: [003] 13 {InlinedFunction} not_inlined 'InlineFunction' -> 'int'
+; ONE-NEXT: [003] 13 {InlinedFunction} inlined 'InlineFunction' -> 'int'
; ONE-NEXT: [004] {Block}
; ONE-NEXT: [005] {Variable} 'Var_2' -> 'int'
; ONE-NEXT: [004] {Parameter} 'Param' -> 'int'
|
pogo59
reviewed
Nov 13, 2024
Collaborator
pogo59
left a comment
pogo59
left a comment
There was a problem hiding this comment.
The test makes it pretty clear this is functionally the correct thing to do. But it covers only the inline attribute; are we missing tests for the other affected attributes? (I can't remember whether there is a unittest for the reader, but if there is, that would seem like a good place to cover all these cases.)
Member
Author
|
We have unittest for the reader: |
- In the DWARF reader, for those attributes that can have an unsigned value, allow for the following cases: * Is an implicit constant * Is an optional value - The testing is done by creating a file with generated DWARF, using 'DwarfGenerator' (generate DWARF debug info for unit tests).
Member
Author
|
@pogo59 Using the existing |
- In the DWARF reader, for those attributes that can have an unsigned value, allow for the following cases: * Is an implicit constant * Is an optional value - The testing is done by creating a file with generated DWARF, using 'DwarfGenerator' (generate DWARF debug info for unit tests). - Remove not needed include header file.
pogo59
reviewed
Nov 26, 2024
|
|
||
| // Helper function to get the first compile unit. | ||
| LVScopeCompileUnit *getFirstCompileUnit(LVScopeRoot *Root) { | ||
| EXPECT_NE(Root, nullptr); |
Collaborator
There was a problem hiding this comment.
Suggested change
| EXPECT_NE(Root, nullptr); | |
| ASSERT_NE(Root, nullptr); |
Use the ASSERT_* macros when the test will later crash if the condition is not true. This is especially the case when verifying a pointer is non-null and will be dereferenced.
Member
Author
There was a problem hiding this comment.
Changed all EXPECT_NE to ASSERT_NE.
Note: There is a restriction on using EXPECT_NE inside a non-void function
https://chromium.googlesource.com/external/github.com/google/googletest/+/HEAD/docs/advanced.md#assertion-placement
For the non-void functions: getFirstCompileUnit and createReader, I used standard if checks and the caller will use ASSERT_NE on the returned value.
| // Check the logical elements basic properties. | ||
| void checkElementAttributes(LVReader *Reader) { | ||
| LVScopeRoot *Root = Reader->getScopesRoot(); | ||
| EXPECT_NE(Root, nullptr); |
Collaborator
There was a problem hiding this comment.
This is redundant with a check inside getFirstCompileUnit
Member
Author
There was a problem hiding this comment.
The check has been removed from getFirstCompileUnit. See previous comment about assertion-placement.
| LVScopeCompileUnit *getFirstCompileUnit(LVScopeRoot *Root) { | ||
| EXPECT_NE(Root, nullptr); | ||
| const LVScopes *CompileUnits = Root->getScopes(); | ||
| EXPECT_NE(CompileUnits, nullptr); |
Collaborator
There was a problem hiding this comment.
Suggested change
| EXPECT_NE(CompileUnits, nullptr); | |
| ASSERT_NE(CompileUnits, nullptr); |
Member
Author
There was a problem hiding this comment.
The check has been removed from getFirstCompileUnit. See previous comment about assertion-placement.
| LVScopes::const_iterator ScopeIter = Scopes->begin(); | ||
| EXPECT_NE(ScopeIter, nullptr); | ||
| LVScope *Scope = static_cast<LVScope *>(*ScopeIter); | ||
| EXPECT_NE(Scope, nullptr); |
Collaborator
There was a problem hiding this comment.
Suggested change
| EXPECT_NE(Scope, nullptr); | |
| ASSERT_NE(Scope, nullptr); |
| EXPECT_EQ(Scope->getDiscriminator(), 8); // ScopeFunctionInlined | ||
|
|
||
| // Check no-values. | ||
| EXPECT_NE(++ScopeIter, nullptr); |
| EXPECT_EQ(Scope->getDiscriminator(), 0); // ScopeFunctionInlined | ||
|
|
||
| // Check implicit values. | ||
| EXPECT_NE(++ScopeIter, nullptr); |
|
|
||
| // Check values. | ||
| LVSymbols::const_iterator SymbolIter = Symbols->begin(); | ||
| EXPECT_NE(SymbolIter, nullptr); |
Collaborator
There was a problem hiding this comment.
Suggested change
| EXPECT_NE(SymbolIter, nullptr); | |
| ASSERT_NE(SymbolIter, nullptr); |
| LVSymbols::const_iterator SymbolIter = Symbols->begin(); | ||
| EXPECT_NE(SymbolIter, nullptr); | ||
| LVSymbol *Symbol = static_cast<LVSymbol *>(*SymbolIter); | ||
| EXPECT_NE(Symbol, nullptr); |
Collaborator
There was a problem hiding this comment.
Suggested change
| EXPECT_NE(Symbol, nullptr); | |
| ASSERT_NE(Symbol, nullptr); |
| EXPECT_EQ(Symbol->getBitSize(), 1); // Symbol | ||
|
|
||
| // Check no-values. | ||
| EXPECT_NE(++SymbolIter, nullptr); |
Collaborator
There was a problem hiding this comment.
This pattern continues, I won't add comments on all of them.
Member
Author
There was a problem hiding this comment.
Removed the repeated comments.
|
|
||
| add_llvm_unittest_with_input_files(DebugInfoLogicalViewTests | ||
| ../DWARF/DwarfGenerator.cpp | ||
| ../DWARF/DwarfUtils.cpp |
Collaborator
There was a problem hiding this comment.
This is okay for now, but really these components should be moved into their own library so they can be shared between unittests.
Member
Author
There was a problem hiding this comment.
That is a good point. I created a PR to record that request. #117864
- In the DWARF reader, for those attributes that can have an
unsigned value, allow for the following cases:
* Is an implicit constant
* Is an optional value
- The testing is done by creating a file with generated DWARF,
using 'DwarfGenerator' (generate DWARF debug info for unit tests).
- Address reviewer comments:
* Changed 'EXPECT_NE' to 'ASSERT_NE'
* Test iterators against 'end()'
* Remove redundant comments
* As the 'ASSERT_NE' can not be used inside a non-void function,
use normal logic and move the checks outside the function.
The caller will use 'ASSERT_NE' on the returned value.
Member
Author
|
@pogo59 Thanks very much for your valuable review. |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/9182 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/9184 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/5999 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/76/builds/4875 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/145/builds/3474 Here is the relevant piece of the build log for the reference |
Member
Author
|
Looking at the reported failures on those specific builders. Link errors
Compile errors: error: comparison of integers of different signs:
PR that fixes the issues: #117971 |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/64/builds/1565 Here is the relevant piece of the build log for the reference |
nico
reviewed
Nov 29, 2024
| @@ -1,17 +1,22 @@ | |||
| set(LLVM_LINK_COMPONENTS | |||
| ${LLVM_TARGETS_TO_BUILD} | |||
Contributor
There was a problem hiding this comment.
Doesn't this make the next 3 lines superfluous?
Member
Author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
DwarfGenerator(generate DWARF debug info for unit tests).