This is an archive of the discontinued LLVM Phabricator instance.

Differential D96831

[obj2yaml,yaml2obj] Add NumBlocks to the BBAddrMapEntry yaml field.
ClosedPublic

Authored by rahmanl on Feb 16 2021, 6:56 PM.

Details

Reviewers
jhenderson
grimar
Commits
rG9f527086609e: [obj2yaml,yaml2obj] Add NumBlocks to the BBAddrMapEntry yaml field.
rG0252e6ead192: [obj2yaml,yaml2obj] Add NumBlocks to the BBAddrMapEntry yaml field.
Summary

As discussed in D95511, this allows us to encode invalid BBAddrMap
sections to be used in more rigorous testing.

Diff Detail

Event Timeline

rahmanl created this revision.Feb 16 2021, 6:56 PM
Herald added subscribers: hiraditya, emaste. · View Herald TranscriptFeb 16 2021, 6:56 PM
rahmanl requested review of this revision.Feb 16 2021, 6:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2021, 6:56 PM
Herald added subscribers: llvm-commits, MaskRay. · View Herald Transcript
rahmanl mentioned this in D95511: [llvm-readelf] Support dumping the BB address map section with --bb-addr-map..Feb 16 2021, 7:01 PM
Harbormaster completed remote builds in B89468: Diff 324164.Feb 16 2021, 8:15 PM
jhenderson accepted this revision.Feb 17 2021, 12:47 AM

LGTM, with nits.

llvm/lib/ObjectYAML/ELFEmitter.cpp
1362
llvm/test/tools/obj2yaml/ELF/bb-addr-map.yaml
133
llvm/tools/obj2yaml/elf2yaml.cpp
864

clang-format seems to recognise this comment styling, and it seems clearer to me.

This revision is now accepted and ready to land.Feb 17 2021, 12:47 AM
rahmanl updated this revision to Diff 324446.Feb 17 2021, 3:35 PM
rahmanl marked 3 inline comments as done.
  • Typos and format.
rahmanl updated this revision to Diff 324449.Feb 17 2021, 3:38 PM
  • rebase.
This revision was landed with ongoing or failed builds.Feb 17 2021, 3:45 PM
Closed by commit rG0252e6ead192: [obj2yaml,yaml2obj] Add NumBlocks to the BBAddrMapEntry yaml field. (authored by rahmanl). · Explain Why
This revision was automatically updated to reflect the committed changes.
rahmanl added a commit: rG0252e6ead192: [obj2yaml,yaml2obj] Add NumBlocks to the BBAddrMapEntry yaml field..
Harbormaster completed remote builds in B89622: Diff 324446.Feb 17 2021, 4:53 PM
Harbormaster completed remote builds in B89624: Diff 324449.Feb 17 2021, 5:27 PM
jhenderson added a comment.Feb 22 2021, 1:10 AM

It looks like you somehow pushed an older version of this change? Several of the typos/minor changes you fixed in previous iterations have been undone. Please take a look and fix accordingly.

I used this link to spot differences that look bad to me: https://reviews.llvm.org/D96831?vs=324449&id=324451#toc

llvm/lib/ObjectYAML/ELFYAML.cpp
1668

This line probably should be above the BBEntries line, right?

llvm/test/tools/obj2yaml/ELF/bb-addr-map.yaml
133

I don't think this typo (the missing "the" before "section") has been fixed.

llvm/tools/obj2yaml/elf2yaml.cpp
864

Similarly, this issue hasn't been addressed in the actual commit that appears on github.

rahmanl reopened this revision.Feb 22 2021, 5:46 PM
This revision is now accepted and ready to land.Feb 22 2021, 5:46 PM
rahmanl updated this revision to Diff 325628.Feb 22 2021, 5:50 PM
  • Typos and formatting.
rahmanl added a comment.Feb 22 2021, 5:52 PM
In D96831#2578436, @jhenderson wrote:

It looks like you somehow pushed an older version of this change? Several of the typos/minor changes you fixed in previous iterations have been undone. Please take a look and fix accordingly.

I used this link to spot differences that look bad to me: https://reviews.llvm.org/D96831?vs=324449&id=324451#toc

My bad, Not sure how this happened. I am pushing the version with fixes now.

rahmanl updated this revision to Diff 325630.Feb 22 2021, 6:01 PM
  • Typos and formatting.
rahmanl updated this revision to Diff 325632.Feb 22 2021, 6:02 PM
  • Typos and formatting.
This revision was landed with ongoing or failed builds.Feb 22 2021, 6:09 PM
Closed by commit rG9f527086609e: [obj2yaml,yaml2obj] Add NumBlocks to the BBAddrMapEntry yaml field. (authored by rahmanl). · Explain Why
This revision was automatically updated to reflect the committed changes.
rahmanl added a commit: rG9f527086609e: [obj2yaml,yaml2obj] Add NumBlocks to the BBAddrMapEntry yaml field..
Harbormaster completed remote builds in B90306: Diff 325628.Feb 22 2021, 6:36 PM
Harbormaster completed remote builds in B90307: Diff 325630.Feb 22 2021, 6:51 PM
Harbormaster completed remote builds in B90308: Diff 325632.

Revision Contents

PathSize
llvm/
lib/
ObjectYAML/
2 lines
2 lines
test/
tools/
obj2yaml/
ELF/
4 lines
tools/
obj2yaml/
2 lines

Diff 325633

llvm/lib/ObjectYAML/ELFEmitter.cpp

Show First 20 Lines • Show All 1,353 Lines • ▼ Show 20 Linesvoid ELFState<ELFT>::writeSectionContent(
ContiguousBlobAccumulator &CBA) { ContiguousBlobAccumulator &CBA) {
if (!Section.Entries) if (!Section.Entries)
return; return;
for (const ELFYAML::BBAddrMapEntry &E : *Section.Entries) { for (const ELFYAML::BBAddrMapEntry &E : *Section.Entries) {
// Write the address of the function. // Write the address of the function.
CBA.write<uintX_t>(E.Address, ELFT::TargetEndianness); CBA.write<uintX_t>(E.Address, ELFT::TargetEndianness);
// Write number of BBEntries (number of basic blocks in the function). This // Write number of BBEntries (number of basic blocks in the function). This
// is overriden by the 'NumBlocks' YAML field if specified. // is overridden by the 'NumBlocks' YAML field when specified.
jhendersonUnsubmitted
Done
ReplyInline Actions
// Write number of BBEntries (number of basic blocks in the function). This
- // is overriden by the 'NumBlocks' YAML field if specified.
+ // is overridden by the 'NumBlocks' YAML field if specified.
uint32_t NumBlocks =
jhenderson:
uint32_t NumBlocks = uint32_t NumBlocks =
E.NumBlocks.getValueOr(E.BBEntries ? E.BBEntries->size() : 0); E.NumBlocks.getValueOr(E.BBEntries ? E.BBEntries->size() : 0);
SHeader.sh_size += sizeof(uintX_t) + CBA.writeULEB128(NumBlocks); SHeader.sh_size += sizeof(uintX_t) + CBA.writeULEB128(NumBlocks);
// Write all BBEntries. // Write all BBEntries.
if (!E.BBEntries) if (!E.BBEntries)
continue; continue;
for (const ELFYAML::BBAddrMapEntry::BBEntry &BBE : *E.BBEntries) for (const ELFYAML::BBAddrMapEntry::BBEntry &BBE : *E.BBEntries)
SHeader.sh_size += CBA.writeULEB128(BBE.AddressOffset) + SHeader.sh_size += CBA.writeULEB128(BBE.AddressOffset) +
▲ Show 20 Lines • Show All 587 Lines • Show Last 20 Lines

llvm/lib/ObjectYAML/ELFYAML.cpp

Show First 20 Lines • Show All 1,659 Lines • ▼ Show 20 Linesvoid MappingTraits<ELFYAML::StackSizeEntry>::mapping(
IO.mapOptional("Address", E.Address, Hex64(0)); IO.mapOptional("Address", E.Address, Hex64(0));
IO.mapRequired("Size", E.Size); IO.mapRequired("Size", E.Size);
} }
void MappingTraits<ELFYAML::BBAddrMapEntry>::mapping( void MappingTraits<ELFYAML::BBAddrMapEntry>::mapping(
IO &IO, ELFYAML::BBAddrMapEntry &E) { IO &IO, ELFYAML::BBAddrMapEntry &E) {
assert(IO.getContext() && "The IO context is not initialized"); assert(IO.getContext() && "The IO context is not initialized");
IO.mapOptional("Address", E.Address, Hex64(0)); IO.mapOptional("Address", E.Address, Hex64(0));
IO.mapOptional("BBEntries", E.BBEntries);
IO.mapOptional("NumBlocks", E.NumBlocks); IO.mapOptional("NumBlocks", E.NumBlocks);
jhendersonUnsubmitted
Not Done
ReplyInline Actions

This line probably should be above the BBEntries line, right?

jhenderson: This line probably should be above the `BBEntries` line, right?
IO.mapOptional("BBEntries", E.BBEntries);
} }
void MappingTraits<ELFYAML::BBAddrMapEntry::BBEntry>::mapping( void MappingTraits<ELFYAML::BBAddrMapEntry::BBEntry>::mapping(
IO &IO, ELFYAML::BBAddrMapEntry::BBEntry &E) { IO &IO, ELFYAML::BBAddrMapEntry::BBEntry &E) {
assert(IO.getContext() && "The IO context is not initialized"); assert(IO.getContext() && "The IO context is not initialized");
IO.mapRequired("AddressOffset", E.AddressOffset); IO.mapRequired("AddressOffset", E.AddressOffset);
IO.mapRequired("Size", E.Size); IO.mapRequired("Size", E.Size);
IO.mapRequired("Metadata", E.Metadata); IO.mapRequired("Metadata", E.Metadata);
▲ Show 20 Lines • Show All 136 Lines • Show Last 20 Lines

llvm/test/tools/obj2yaml/ELF/bb-addr-map.yaml

Show First 20 Lines • Show All 124 Lines • ▼ Show 20 Lines## Check that obj2yaml does not emit the Address field when it's zero.
Size: 0x2 Size: 0x2
Metadata: 0x3 Metadata: 0x3
- Name: '.llvm_bb_addr_map (1)' - Name: '.llvm_bb_addr_map (1)'
Type: SHT_LLVM_BB_ADDR_MAP Type: SHT_LLVM_BB_ADDR_MAP
Entries: Entries:
- Address: 0x20 - Address: 0x20
## Check that obj2yaml uses the "Content" tag to describe an .llvm_bb_addr_map section ## Check that obj2yaml uses the "Content" tag to describe an .llvm_bb_addr_map section
## when it can't extract the entries, for example, when section is truncated, or when ## when it can't extract the entries, for example, when the section is truncated, or
jhendersonUnsubmitted
Done
ReplyInline Actions
## Check that obj2yaml uses the "Content" tag to describe an .llvm_bb_addr_map section
- ## when it can't extract the entries, for example, when section is truncated, or when
+ ## when it can't extract the entries, for example, when the section is truncated, or when
## an invalid NumBlocks field is specified.
jhenderson:
jhendersonUnsubmitted
Not Done
ReplyInline Actions

I don't think this typo (the missing "the" before "section") has been fixed.

jhenderson: I don't think this typo (the missing "the" before "section") has been fixed.
## an invalid NumBlocks field is specified. ## when an invalid 'NumBlocks' field is specified.
# RUN: yaml2obj --docnum=1 -DSIZE=0x8 %s -o %t4 # RUN: yaml2obj --docnum=1 -DSIZE=0x8 %s -o %t4
# RUN: obj2yaml %t4 | FileCheck %s --check-prefixes=TRUNCATED,INVALID # RUN: obj2yaml %t4 | FileCheck %s --check-prefixes=TRUNCATED,INVALID
# RUN: yaml2obj --docnum=1 -DNUMBLOCKS=2 %s -o %t5 # RUN: yaml2obj --docnum=1 -DNUMBLOCKS=2 %s -o %t5
# RUN: obj2yaml %t5 | FileCheck %s --check-prefixes=BADNUMBLOCKS,INVALID # RUN: obj2yaml %t5 | FileCheck %s --check-prefixes=BADNUMBLOCKS,INVALID
# INVALID: --- !ELF # INVALID: --- !ELF
Show All 9 Lines

llvm/tools/obj2yaml/elf2yaml.cpp

Show First 20 Lines • Show All 855 Lines • ▼ Show 20 Lines while (Cur && Cur.tell() < Content.size()) {
std::vector<ELFYAML::BBAddrMapEntry::BBEntry> BBEntries; std::vector<ELFYAML::BBAddrMapEntry::BBEntry> BBEntries;
// Read the specified number of BB entries, or until decoding fails. // Read the specified number of BB entries, or until decoding fails.
for (uint64_t BlockID = 0; Cur && BlockID < NumBlocks; ++BlockID) { for (uint64_t BlockID = 0; Cur && BlockID < NumBlocks; ++BlockID) {
uint64_t Offset = Data.getULEB128(Cur); uint64_t Offset = Data.getULEB128(Cur);
uint64_t Size = Data.getULEB128(Cur); uint64_t Size = Data.getULEB128(Cur);
uint64_t Metadata = Data.getULEB128(Cur); uint64_t Metadata = Data.getULEB128(Cur);
BBEntries.push_back({Offset, Size, Metadata}); BBEntries.push_back({Offset, Size, Metadata});
} }
Entries.push_back({Address, /* NumBlocks */ {}, BBEntries}); Entries.push_back({Address, /*NumBlocks=*/{}, BBEntries});
jhendersonUnsubmitted
Done
ReplyInline Actions
BBEntries.push_back({Offset, Size, Metadata});
}
- Entries.push_back({Address, /* NumBlocks */ {}, BBEntries});
+ Entries.push_back({Address, /*NumBlocks=*/{}, BBEntries});
}
if (!Cur) {

clang-format seems to recognise this comment styling, and it seems clearer to me.

jhenderson: clang-format seems to recognise this comment styling, and it seems clearer to me.
jhendersonUnsubmitted
Not Done
ReplyInline Actions

Similarly, this issue hasn't been addressed in the actual commit that appears on github.

jhenderson: Similarly, this issue hasn't been addressed in the actual commit that appears on github.
} }
if (!Cur) { if (!Cur) {
// If the section cannot be decoded, we dump it as an array of bytes. // If the section cannot be decoded, we dump it as an array of bytes.
consumeError(Cur.takeError()); consumeError(Cur.takeError());
S->Content = yaml::BinaryRef(Content); S->Content = yaml::BinaryRef(Content);
} else { } else {
S->Entries = std::move(Entries); S->Entries = std::move(Entries);
▲ Show 20 Lines • Show All 696 Lines • Show Last 20 Lines