[PATCH] D112465: [llvm-profgen] Avoid writing any data to CSNameTableSection for the empty CSNameTable

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/lib/ProfileData/SampleProfWriter.cpp:255-256
 std::error_code SampleProfileWriterExtBinaryBase::writeCSNameTableSection() {
+  if (CSNameTable.empty())
+    return sampleprof_error::success;
+
----------------
hoy wrote:
> wenlei wrote:
> > wlei wrote:
> > > wenlei wrote:
> > > > wlei wrote:
> > > > > wenlei wrote:
> > > > > > `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.
> > > > > 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).
> > > > > Like the ProfileSymbolList, it can be a zero section size:
> > > > > ```
> > > > >   if (ProfSymList && ProfSymList->size() > 0)
> > > > >     if (std::error_code EC = ProfSymList->write(*OutputStream))
> > > > >       return EC;
> > > > > 
> > > > >   return sampleprof_error::success;
> > > > > ```
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > 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).
> > > > 
> > > >  
> > > 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 
> > Porting that patch back sounds like a better solution to me. Thanks for looking into it!
> Sounds better to port over that change. Thanks for figuring that out!
Thanks and sorry for the back and forth.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112465/new/

https://reviews.llvm.org/D112465