Skip to content

Commit

Permalink
Fix bolt for #98905
Browse files Browse the repository at this point in the history
  • Loading branch information
@labath
labath committed Jul 16, 2024
1 parent 4abdb85 commit 9dab912
Showing 1 changed file with 18 additions and 2 deletions.
20 changes: 18 additions & 2 deletions bolt/lib/Core/DIEBuilder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,17 @@ DWARFDie DIEBuilder::resolveDIEReference(
const DWARFAbbreviationDeclaration::AttributeSpec AttrSpec,
DWARFUnit *&RefCU, DWARFDebugInfoEntry &DwarfDebugInfoEntry) {
assert(RefValue.isFormClass(DWARFFormValue::FC_Reference));
uint64_t RefOffset = *RefValue.getAsReference();
uint64_t RefOffset;
if (std::optional<uint64_t> Off = RefValue.getAsRelativeReference()) {
RefOffset = RefValue.getUnit()->getOffset() + *Off;
} else if (Off = RefValue.getAsDebugInfoReference(); Off) {
RefOffset = *Off;
} else {
BC.errs()
<< "BOLT-WARNING: [internal-dwarf-error]: unsupported reference type: "
<< FormEncodingString(RefValue.getForm()) << ".\n";
return DWARFDie();
}
return resolveDIEReference(AttrSpec, RefOffset, RefCU, DwarfDebugInfoEntry);
}

Expand Down Expand Up @@ -607,7 +617,13 @@ void DIEBuilder::cloneDieReferenceAttribute(
DIE &Die, const DWARFUnit &U, const DWARFDie &InputDIE,
const DWARFAbbreviationDeclaration::AttributeSpec AttrSpec,
const DWARFFormValue &Val) {
const uint64_t Ref = *Val.getAsReference();
uint64_t Ref;
if (std::optional<uint64_t> Off = Val.getAsRelativeReference())
Ref = Val.getUnit()->getOffset() + *Off;
else if (Off = Val.getAsDebugInfoReference(); Off)
Ref = *Off;
else
return;

DIE *NewRefDie = nullptr;
DWARFUnit *RefUnit = nullptr;
Expand Down

4 comments on commit 9dab912

@ayermolo
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think a warning in resolveDIEReference will trigger, because it will exit in cloneDieReferenceAttribute.
Also what would be the test that would trigger this?

@labath
Copy link
Collaborator Author

@labath labath commented on 9dab912 Jul 16, 2024

Choose a reason for hiding this comment

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

I don't know how bolt works (I just tried to make a conservative change), but the simplest way to generate type signature references is to use clang's -fdebug-types-section flag. bin/clang -c -x c++ -o - - -g -fdebug-types-section <<<"struct A{} a;" | llvm-dwarfdump - -debug-info -v will generate a reference to A like this:

0x00000029:   DW_TAG_structure_type [5]   (0x0000000c)
                DW_AT_declaration [DW_FORM_flag_present]	(true)
                DW_AT_signature [DW_FORM_ref_sig8]	(0x4e834ea939695c24)

Having said that, I now realize this only applies to type references, so if this code is never called on type references then there may not be a way to trigger it, and maybe this should be an assert instead (although you could still probably hit it with invalid/malformed dwarf -- I don't know what's bolt's policy here).

"supplementary" references are more elusive, and I don't think there's a way generate them using the llvm toolchain (the dwz tool might do that). To the best of my knowledge, no llvm tool (producer or consumer) actually supports working with these kinds of files, but in theory they could be used for almost any reference.

@ayermolo
Copy link
Contributor

Choose a reason for hiding this comment

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

It does get called on type references, and we do have tests for it. I was referring to the else case that triggers a warning.
So sounds like that might get generated by dwz tool.
In the future please create a PR for your changes.

BOLT policy has evolved. Generally speaking we try to continue as much as possible while printing a warning, as not to block builds with BOLT failures related to debug information.

@labath
Copy link
Collaborator Author

@labath labath commented on 9dab912 Jul 17, 2024

Choose a reason for hiding this comment

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

It does get called on type references, and we do have tests for it. I was referring to the else case that triggers a warning. So sounds like that might get generated by dwz tool. In the future please create a PR for your changes.

I was trying to fix the build breakage quickly and I got careless. Point taken.

Looking at this with a cooler head, I think I have now have a better fix. Please take a look at #99324.

(This also explains why this works with type units even though the original code was clearly broken for type units -- the function I changed really was not called for type unit. It's also not called for supplementary references, as those wouldn't make it past cloneAttribute).

BOLT policy has evolved. Generally speaking we try to continue as much as possible while printing a warning, as not to block builds with BOLT failures related to debug information.

Ack.

Please sign in to comment.