We can leave it zero size of when CSNameTable is empty. Previously it will write the size(value is zero but it has 1 byte) to this section.
This is for the backward compatibility as extbinary only skip the zero size for the unknown section.
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/lib/ProfileData/SampleProfWriter.cpp | ||
|---|---|---|
| 255–256 | SampleProfileReaderExtBinaryBase::readCSNameTableSec would still expect to read a zero here, and if we skip writing the zero, the read will get some garbage data? Or did I miss anything? Also this is inconsistent with how we write other sections - we don't omit zero size. | |
| llvm/lib/ProfileData/SampleProfWriter.cpp | ||
|---|---|---|
| 255–256 | You see the header reader logic below, at the beginning of the loop, it will skip the zero size entry ( if (!Entry.Size)). so it won't call the readCSNameTableSec. Also it has the sanity check after read(if (Data != SecStart + SecSize)) for (auto &Entry : SecHdrTable) {
// Skip empty section.
if (!Entry.Size)
continue;
...
if (std::error_code EC = readOneSection(SecStart, SecSize, Entry))
return EC;
if (Data != SecStart + SecSize)
return sampleprof_error::malformed;
....I feel like each section doesn't follow strong encoding convention. NameTable is encoded the size probably because it won't have a zero size(if the profile is not empty). if (ProfSymList && ProfSymList->size() > 0)
if (std::error_code EC = ProfSymList->write(*OutputStream))
return EC;
return sampleprof_error::success; | |
| llvm/lib/ProfileData/SampleProfWriter.cpp | ||
|---|---|---|
| 255–256 | Ok, the check on Entry.Size explains how it works. But the asymmetry between readCSNameTableSec and writeCSNameTableSection still isn't ideal. One is expecting a size unconditionally, while the other doesn't always write a size. The profile symbol list is different in the sense that it never has a size field anyways, so there's no inconsistency. I guess practically it works, so if we can't have a better solution, we can do it. If we do this, perhaps add a check to make it explicit that we don't expect zero size on the reader side (SampleProfileReaderExtBinaryBase::readCSNameTableSec). | |
| llvm/lib/ProfileData/SampleProfWriter.cpp | ||
|---|---|---|
| 255–256 | OK, I think you concern is decent. After more digging, I actually found this https://reviews.llvm.org/D109398, this is to solve the same forward compatibility issue we encountered. Then I guess we can just port this to our clang-9? cc: @hoy | |
| llvm/lib/ProfileData/SampleProfWriter.cpp | ||
|---|---|---|
| 255–256 | Porting that patch back sounds like a better solution to me. Thanks for looking into it! | |
| llvm/lib/ProfileData/SampleProfWriter.cpp | ||
|---|---|---|
| 255–256 | Sounds better to port over that change. Thanks for figuring that out! | |
| llvm/lib/ProfileData/SampleProfWriter.cpp | ||
|---|---|---|
| 255–256 | Thanks and sorry for the back and forth. | |
SampleProfileReaderExtBinaryBase::readCSNameTableSec would still expect to read a zero here, and if we skip writing the zero, the read will get some garbage data? Or did I miss anything?
Also this is inconsistent with how we write other sections - we don't omit zero size.