This is an archive of the discontinued LLVM Phabricator instance.

Differential D120528

[BOLT][DWARF] Fix how DW_AT_high_pc [DW_FORM_udata] is handled
ClosedPublic

Authored by ayermolo on Feb 24 2022, 5:11 PM.

Details

Reviewers
rafauler
Amir
maksfb
jdoerfert
Commits
rGa44fe31977c7: [BOLT][DWARF] Fix how DW_AT_high_pc [DW_FORM_udata] is handled
Summary

We were not handling correctly conversion from DW_AT_high_pc into DW_AT_ranges,
when size of DW_AT_high_pc is not 4/8 bytes.

Diff Detail

Event Timeline

ayermolo created this revision.Feb 24 2022, 5:11 PM
Herald added a reviewer: rafauler. · View Herald TranscriptFeb 24 2022, 5:11 PM
Herald added a reviewer: Amir. · View Herald Transcript
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added subscribers: hoy, modimo, wenlei. · View Herald Transcript
ayermolo requested review of this revision.Feb 24 2022, 5:11 PM
Herald added a reviewer: jdoerfert. · View Herald TranscriptFeb 24 2022, 5:11 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, yota9, sstefan1. · View Herald Transcript
Harbormaster completed remote builds in B151384: Diff 411273.Feb 24 2022, 5:20 PM
ayermolo updated this revision to Diff 411278.Feb 24 2022, 5:28 PM

small fix

Harbormaster completed remote builds in B151388: Diff 411278.Feb 24 2022, 5:37 PM
maksfb added inline comments.Feb 24 2022, 6:01 PM
bolt/include/bolt/Core/DebugData.h
540

Could you add a description of the patch kind in a comment?

546

I don't see the need to redefine getKind().

ayermolo updated this revision to Diff 411298.Feb 24 2022, 8:16 PM

address comments

ayermolo marked 2 inline comments as done.Feb 24 2022, 8:17 PM
Harbormaster completed remote builds in B151403: Diff 411298.Feb 24 2022, 8:23 PM
maksfb accepted this revision.Feb 25 2022, 7:52 AM

nit: drop the trailing period from the title.

This revision is now accepted and ready to land.Feb 25 2022, 7:52 AM
ayermolo retitled this revision from [BOLT][DWARF] Fix how DW_AT_high_pc [DW_FORM_udata] is handled. to [BOLT][DWARF] Fix how DW_AT_high_pc [DW_FORM_udata] is handled.Feb 25 2022, 10:26 AM
Closed by commit rGa44fe31977c7: [BOLT][DWARF] Fix how DW_AT_high_pc [DW_FORM_udata] is handled (authored by ayermolo). · Explain WhyFeb 25 2022, 10:32 AM
This revision was automatically updated to reflect the committed changes.
ayermolo added a commit: rGa44fe31977c7: [BOLT][DWARF] Fix how DW_AT_high_pc [DW_FORM_udata] is handled.

Revision Contents

PathSize
bolt/
include/
bolt/
Core/
19 lines
lib/
Core/
19 lines
test/
X86/
150 lines

Diff 411278

bolt/include/bolt/Core/DebugData.h

Show First 20 Lines • Show All 484 Lines • ▼ Show 20 Lines
}; };
class DebugInfoBinaryPatcher : public SimpleBinaryPatcher { class DebugInfoBinaryPatcher : public SimpleBinaryPatcher {
public: public:
enum class DebugPatchKind { enum class DebugPatchKind {
PatchBaseClass, PatchBaseClass,
PatchValue32, PatchValue32,
PatchValue64to32, PatchValue64to32,
PatchValue32GenericSize,
PatchValue64, PatchValue64,
PatchValueVariable, PatchValueVariable,
ReferencePatchValue, ReferencePatchValue,
DWARFUnitOffsetBaseLabel, DWARFUnitOffsetBaseLabel,
DestinationReferenceLabel, DestinationReferenceLabel,
NewDebugEntry NewDebugEntry
}; };
Show All 30 Lines struct DebugPatch32 : public Patch {
DebugPatchKind getKind() const { return Kind; } DebugPatchKind getKind() const { return Kind; }
static bool classof(const Patch *Writer) { static bool classof(const Patch *Writer) {
return Writer->getKind() == DebugPatchKind::PatchValue32; return Writer->getKind() == DebugPatchKind::PatchValue32;
} }
uint32_t Value; uint32_t Value;
}; };
struct DebugPatch32GenericSize : public Patch {
maksfbUnsubmitted
Done
ReplyInline Actions

Could you add a description of the patch kind in a comment?

maksfb: Could you add a description of the patch kind in a comment?
DebugPatch32GenericSize(uint32_t O, uint32_t V, uint32_t OVS)
: Patch(O, DebugPatchKind::PatchValue32GenericSize) {
Value = V;
OldValueSize = OVS;
}
DebugPatchKind getKind() const { return Kind; }
maksfbUnsubmitted
Done
ReplyInline Actions

I don't see the need to redefine getKind().

maksfb: I don't see the need to redefine `getKind()`.
static bool classof(const Patch *Writer) {
return Writer->getKind() == DebugPatchKind::PatchValue32GenericSize;
}
uint32_t Value;
uint32_t OldValueSize;
};
struct DebugPatch64 : public Patch { struct DebugPatch64 : public Patch {
DebugPatch64(uint32_t O, uint64_t V) DebugPatch64(uint32_t O, uint64_t V)
: Patch(O, DebugPatchKind::PatchValue64) { : Patch(O, DebugPatchKind::PatchValue64) {
Value = V; Value = V;
} }
static bool classof(const Patch *Writer) { static bool classof(const Patch *Writer) {
return Writer->getKind() == DebugPatchKind::PatchValue64; return Writer->getKind() == DebugPatchKind::PatchValue64;
▲ Show 20 Lines • Show All 141 Lines • ▼ Show 20 Lines void operator()(Patch *P) const {
llvm_unreachable( llvm_unreachable(
"Not all delete instances handled for Patch derived classes"); "Not all delete instances handled for Patch derived classes");
case DebugPatchKind::PatchValue32: case DebugPatchKind::PatchValue32:
delete reinterpret_cast<DebugPatch32 *>(P); delete reinterpret_cast<DebugPatch32 *>(P);
break; break;
case DebugPatchKind::PatchValue64to32: case DebugPatchKind::PatchValue64to32:
delete reinterpret_cast<DebugPatch64to32 *>(P); delete reinterpret_cast<DebugPatch64to32 *>(P);
break; break;
case DebugPatchKind::PatchValue32GenericSize:
delete reinterpret_cast<DebugPatch32GenericSize *>(P);
break;
case DebugPatchKind::PatchValue64: case DebugPatchKind::PatchValue64:
delete reinterpret_cast<DebugPatch64 *>(P); delete reinterpret_cast<DebugPatch64 *>(P);
break; break;
case DebugPatchKind::PatchValueVariable: case DebugPatchKind::PatchValueVariable:
delete reinterpret_cast<DebugPatchVariableSize *>(P); delete reinterpret_cast<DebugPatchVariableSize *>(P);
break; break;
case DebugPatchKind::ReferencePatchValue: case DebugPatchKind::ReferencePatchValue:
delete reinterpret_cast<DebugPatchReference *>(P); delete reinterpret_cast<DebugPatchReference *>(P);
▲ Show 20 Lines • Show All 252 Lines • Show Last 20 Lines

bolt/lib/Core/DebugData.cpp

Show First 20 Lines • Show All 480 Lines • ▼ Show 20 Lines
} }
void DebugInfoBinaryPatcher::addLE32Patch(uint64_t Offset, uint32_t NewValue, void DebugInfoBinaryPatcher::addLE32Patch(uint64_t Offset, uint32_t NewValue,
uint32_t OldValueSize) { uint32_t OldValueSize) {
Offset -= DWPUnitOffset; Offset -= DWPUnitOffset;
std::lock_guard<std::mutex> Lock(WriterMutex); std::lock_guard<std::mutex> Lock(WriterMutex);
if (OldValueSize == 4) if (OldValueSize == 4)
DebugPatches.emplace_back(new DebugPatch32(Offset, NewValue)); DebugPatches.emplace_back(new DebugPatch32(Offset, NewValue));
else else if (OldValueSize == 8)
DebugPatches.emplace_back(new DebugPatch64to32(Offset, NewValue)); DebugPatches.emplace_back(new DebugPatch64to32(Offset, NewValue));
else
DebugPatches.emplace_back(
new DebugPatch32GenericSize(Offset, NewValue, OldValueSize));
} }
void SimpleBinaryPatcher::addBinaryPatch(uint64_t Offset, void SimpleBinaryPatcher::addBinaryPatch(uint64_t Offset,
std::string &&NewValue, std::string &&NewValue,
uint32_t OldValueSize) { uint32_t OldValueSize) {
Patches.emplace_back(Offset, std::move(NewValue)); Patches.emplace_back(Offset, std::move(NewValue));
} }
▲ Show 20 Lines • Show All 74 Lines • ▼ Show 20 Lines for (UniquePatchPtrType &PatchBase : DebugPatches) {
Patch *P = PatchBase.get(); Patch *P = PatchBase.get();
switch (P->Kind) { switch (P->Kind) {
default: default:
continue; continue;
case DebugPatchKind::PatchValue64to32: { case DebugPatchKind::PatchValue64to32: {
PreviousChangeInSize -= 4; PreviousChangeInSize -= 4;
break; break;
} }
case DebugPatchKind::PatchValue32GenericSize: {
DebugPatch32GenericSize *DPVS =
reinterpret_cast<DebugPatch32GenericSize *>(P);
PreviousChangeInSize += 4 - DPVS->OldValueSize;
break;
}
case DebugPatchKind::PatchValueVariable: { case DebugPatchKind::PatchValueVariable: {
DebugPatchVariableSize *DPV = DebugPatchVariableSize *DPV =
reinterpret_cast<DebugPatchVariableSize *>(P); reinterpret_cast<DebugPatchVariableSize *>(P);
std::string Temp; std::string Temp;
raw_string_ostream OS(Temp); raw_string_ostream OS(Temp);
encodeULEB128(DPV->Value, OS); encodeULEB128(DPV->Value, OS);
PreviousChangeInSize += Temp.size() - DPV->OldValueSize; PreviousChangeInSize += Temp.size() - DPV->OldValueSize;
break; break;
▲ Show 20 Lines • Show All 97 Lines • ▼ Show 20 Lines for (const UniquePatchPtrType &PatchBase : DebugPatches) {
} }
case DebugPatchKind::PatchValue64to32: { case DebugPatchKind::PatchValue64to32: {
DebugPatch64to32 *P64to32 = reinterpret_cast<DebugPatch64to32 *>(P); DebugPatch64to32 *P64to32 = reinterpret_cast<DebugPatch64to32 *>(P);
Offset = P64to32->Offset; Offset = P64to32->Offset;
OldValueSize = 8; OldValueSize = 8;
ByteSequence = encodeLE(4, P64to32->Value); ByteSequence = encodeLE(4, P64to32->Value);
break; break;
} }
case DebugPatchKind::PatchValue32GenericSize: {
DebugPatch32GenericSize *DPVS =
reinterpret_cast<DebugPatch32GenericSize *>(P);
Offset = DPVS->Offset;
OldValueSize = DPVS->OldValueSize;
ByteSequence = encodeLE(4, DPVS->Value);
break;
}
case DebugPatchKind::PatchValueVariable: { case DebugPatchKind::PatchValueVariable: {
DebugPatchVariableSize *PV = DebugPatchVariableSize *PV =
reinterpret_cast<DebugPatchVariableSize *>(P); reinterpret_cast<DebugPatchVariableSize *>(P);
Offset = PV->Offset; Offset = PV->Offset;
OldValueSize = PV->OldValueSize; OldValueSize = PV->OldValueSize;
ByteSequence.clear(); ByteSequence.clear();
raw_string_ostream OS(ByteSequence); raw_string_ostream OS(ByteSequence);
encodeULEB128(PV->Value, OS); encodeULEB128(PV->Value, OS);
▲ Show 20 Lines • Show All 540 Lines • Show Last 20 Lines

bolt/test/X86/high_pc_udata.s

  • This file was added.
# REQUIRES: system-linux
# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-linux %s -o %t1.o
# RUN: %clang %cflags %t1.o -o %t.exe -Wl,-q
# RUN: llvm-bolt %t.exe -o %t.bolt -update-debug-sections
# RUN: llvm-dwarfdump --show-form --verbose --debug-info %t.exe | FileCheck --check-prefix=PRECHECK %s
# RUN: llvm-dwarfdump --show-form --verbose --debug-info %t.bolt | FileCheck --check-prefix=POSTCHECK %s
# PRECHECK: DW_AT_high_pc [DW_FORM_udata] (15)
# PRECHECK-NEXT: DW_AT_name [DW_FORM_strp]
# PRECHECK-SAME: "main.cpp"
# POSTCHECK: DW_AT_ranges [DW_FORM_sec_offset]
# POSTCHECK-NEXT: [
# POSTCHECK-NEXT: DW_AT_name [DW_FORM_strp]
# POSTCHECK-SAME: "main.cpp"
# Testing that BOLT transforms DW_AT_high_pc of form DW_FORM_udata correctly into DW_AT_ranges.
# Manually changed so that DW_AT_high_pc is DW_FORM_udata, and that DW_AT_name is after it.
# int main() {
# return 0;
# }
.text
.file "main.cpp"
.globl main # -- Begin function main
.p2align 4, 0x90
.type main,@function
main: # @main
.Lfunc_begin0:
.file 1 "" "main.cpp"
.loc 1 1 0 # main.cpp:1:0
.cfi_startproc
# %bb.0: # %entry
pushq %rbp
.cfi_def_cfa_offset 16
.cfi_offset %rbp, -16
movq %rsp, %rbp
.cfi_def_cfa_register %rbp
movl $0, -4(%rbp)
.Ltmp0:
.loc 1 2 1 prologue_end # main.cpp:2:1
xorl %eax, %eax
popq %rbp
.cfi_def_cfa %rsp, 8
retq
.Ltmp1:
.Lfunc_end0:
.size main, .Lfunc_end0-main
.cfi_endproc
# -- End function
.section .debug_abbrev,"",@progbits
.byte 1 # Abbreviation Code
.byte 17 # DW_TAG_compile_unit
.byte 1 # DW_CHILDREN_yes
.byte 37 # DW_AT_producer
.byte 14 # DW_FORM_strp
.byte 19 # DW_AT_language
.byte 5 # DW_FORM_data2
.byte 16 # DW_AT_stmt_list
.byte 23 # DW_FORM_sec_offset
.byte 27 # DW_AT_comp_dir
.byte 14 # DW_FORM_strp
.byte 17 # DW_AT_low_pc
.byte 1 # DW_FORM_addr
.byte 18 # DW_AT_high_pc
.byte 15 # DW_FORM_udata
.byte 3 # DW_AT_name
.byte 14 # DW_FORM_strp
.byte 0 # EOM(1)
.byte 0 # EOM(2)
.byte 2 # Abbreviation Code
.byte 46 # DW_TAG_subprogram
.byte 0 # DW_CHILDREN_no
.byte 17 # DW_AT_low_pc
.byte 1 # DW_FORM_addr
.byte 18 # DW_AT_high_pc
.byte 6 # DW_FORM_data4
.byte 64 # DW_AT_frame_base
.byte 24 # DW_FORM_exprloc
.byte 3 # DW_AT_name
.byte 14 # DW_FORM_strp
.byte 58 # DW_AT_decl_file
.byte 11 # DW_FORM_data1
.byte 59 # DW_AT_decl_line
.byte 11 # DW_FORM_data1
.byte 73 # DW_AT_type
.byte 19 # DW_FORM_ref4
.byte 63 # DW_AT_external
.byte 25 # DW_FORM_flag_present
.byte 0 # EOM(1)
.byte 0 # EOM(2)
.byte 3 # Abbreviation Code
.byte 36 # DW_TAG_base_type
.byte 0 # DW_CHILDREN_no
.byte 3 # DW_AT_name
.byte 14 # DW_FORM_strp
.byte 62 # DW_AT_encoding
.byte 11 # DW_FORM_data1
.byte 11 # DW_AT_byte_size
.byte 11 # DW_FORM_data1
.byte 0 # EOM(1)
.byte 0 # EOM(2)
.byte 0 # EOM(3)
.section .debug_info,"",@progbits
.Lcu_begin0:
.long .Ldebug_info_end0-.Ldebug_info_start0 # Length of Unit
.Ldebug_info_start0:
.short 4 # DWARF version number
.long .debug_abbrev # Offset Into Abbrev. Section
.byte 8 # Address Size (in bytes)
.byte 1 # Abbrev [1] 0xb:0x40 DW_TAG_compile_unit
.long .Linfo_string0 # DW_AT_producer
.short 33 # DW_AT_language
.long .Lline_table_start0 # DW_AT_stmt_list
.long .Linfo_string2 # DW_AT_comp_dir
.quad .Lfunc_begin0 # DW_AT_low_pc
.byte .Lfunc_end0-.Lfunc_begin0 # DW_AT_high_pc
.long .Linfo_string1 # DW_AT_name
.byte 2 # Abbrev [2] 0x2a:0x19 DW_TAG_subprogram
.quad .Lfunc_begin0 # DW_AT_low_pc
.long .Lfunc_end0-.Lfunc_begin0 # DW_AT_high_pc
.byte 1 # DW_AT_frame_base
.byte 86
.long .Linfo_string3 # DW_AT_name
.byte 1 # DW_AT_decl_file
.byte 1 # DW_AT_decl_line
.long 67 # DW_AT_type
# DW_AT_external
.byte 3 # Abbrev [3] 0x43:0x7 DW_TAG_base_type
.long .Linfo_string4 # DW_AT_name
.byte 5 # DW_AT_encoding
.byte 4 # DW_AT_byte_size
.byte 0 # End Of Children Mark
.Ldebug_info_end0:
.section .debug_str,"MS",@progbits,1
.Linfo_string0:
.asciz "clang" # string offset=0
.Linfo_string1:
.asciz "main.cpp" # string offset=134
.Linfo_string2:
.asciz "test" # string offset=143
.Linfo_string3:
.asciz "main" # string offset=186
.Linfo_string4:
.asciz "int" # string offset=191
.ident "clang"
.section ".note.GNU-stack","",@progbits
.addrsig
.section .debug_line,"",@progbits
.Lline_table_start0: