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

[William Junda Huang via Phabricator via llvm-commits]

huangjd added inline comments.


================
Comment at: llvm/test/tools/llvm-profdata/sample-nametable.test:1
+Test several edge cases with unusual name table data in ExtBinary format.
+
----------------
davidxl wrote:
> 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?
They are all intentionally constructed malformed data and cannot be represented in text form, since the principle is not to trust user input, so the compiler should safeguard against such cases 


================
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
+
----------------
davidxl wrote:
> Should this succeed instead? I assume without the fix it will crash?
The design decision (which I have previously asked for clarification and got no response https://discourse.llvm.org/t/llvm-profdata-what-behavior-was-intended-if-multiple-name-tables-are-present-in-an-extensible-sample-profile/69145) here is to make the reader clear a previously read name table when reading a new one. In this case the second name table only have one entry, so the function sample, being read after reading the second name table, has a string index of 1, which is out of bounds, so it should be rejected. Before the patch, it appends the second name table on the first one, while changing the fixed lenght MD5 base address to the second table, and index (out of bounds) into the sample's section, reading garbage data instead of crashing 


================
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
+
----------------
davidxl wrote:
> Are there any expected strings?
This one is similar to the previous test, also out of bounds when indexing into the second table. In the original code it crashes because the fixed length MD5 base address is not set correctly 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148188