[PATCH] D148188: [llvm-profdata] Make profile reader behavior consistent when encountering malformed profiles

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:745
+    bool FixedLengthMD5 =
         hasSecFlag(Entry, SecNameTableFlags::SecFlagFixedLengthMD5);
+    bool UseMD5 =
----------------
why changing FixedLengthMD5 into a local variable?


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:1314
 
+  bool HasMD5 = false;
+  bool HasNonMD5 = false;
----------------
Should the code be guarded with #ifdef NDEBUG?


================
Comment at: llvm/test/tools/llvm-profdata/sample-nametable.test:1
+Test several edge cases with unusual name table data in ExtBinary format.
+
----------------
Are the test cases (profile data) ill-formed data or valid data? If valid, is it possible to check in the text format and convert into binary format in the testing itself?


================
Comment at: llvm/test/tools/llvm-profdata/sample-nametable.test:4
+1- Multiple fixed-length MD5 name tables. Reading a new table should clear the content from old table, and a valid name index for the old name table should become invalid if the new name table has fewer entries.
+RUN: not llvm-profdata show --sample %p/Inputs/sample-multiple-nametables.profdata
+
----------------
Should this succeed instead? I assume without the fix it will crash?


================
Comment at: llvm/test/tools/llvm-profdata/sample-nametable.test:7
+2- Multiple name tables, the first one has an empty string, the second one tricks the reader into expecting fixed-length MD5 values. Reader should not attempt "lazy loading" of the MD5 string in this case.
+RUN: not llvm-profdata show --sample %p/Inputs/sample-nametable-empty-string.profdata
+
----------------
Are there any expected strings?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148188