[PATCH] D148868: [llvm-profdata] ProfileReader cleanup - preparation for MD5 refactoring

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:1038
   if (FixedLengthMD5) {
+    auto Size = readNumber<size_t>();
+    if (std::error_code EC = Size.getError())
----------------
Add assert(IsMD5).


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:1061
+  if (IsMD5) {
+    auto Size = readNumber<size_t>();
+    if (std::error_code EC = Size.getError())
----------------
add assert (!FixedLengthMD5);


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:714
   case SecNameTable: {
-    FixedLengthMD5 =
+    bool FixedLengthMD5 =
         hasSecFlag(Entry, SecNameTableFlags::SecFlagFixedLengthMD5);
----------------
huangjd wrote:
> davidxl wrote:
> > huangjd wrote:
> > > davidxl wrote:
> > > > Is this flag still used or can be asserted to be true?
> > > It cannot be assumed true. User can specify 3 modes normally:
> > > 1. function name strings : ~SecFlagMD5Name && ~SecFlagFixedLengthMD5
> > > 2. ULEB128 MD5 : SecFlagMD5Name && ~SecFlagFixedLengthMD5
> > > 3. Fixed length MD5 : SecFlagMD5Name && SecFlagFixedLengthMD5
> > > A malformed profile can specify ~SecFlagMD5Name && SecFlagFixedLengthMD5 and crash on the third added test case. I fixed the logic so that LLVM treats this case same as (3). 
> > is 3 the common and the most efficient one?     What is the default with extbinary format?  If 3 is the default,  we should consider deprecate settings.
> It is not default. The default is Binary format, which I propose to switch to ExtBinary first.  
Looking forward to the followup cleanup of binary format and related code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148868