[llvm-profdata] Fix bug llvm-profdata crashes when reading a text sample profile with an empty line with spaces.

[huangjd]

Summary

Text editors can introduce spaces aligning the previous line's indentation. This crashes llvm-profdata. Added check to handle this case.

[snehasish]

lgtm

[ro]

A reghunt just determined that this patch broke a considerable number (30+) of profile-related tests on Solaris/sparcv9. E.g.

+  LLVM :: Other/new-pm-thinlto-prelink-samplepgo-inline-threshold.ll

/vol/llvm/src/llvm-project/dist/llvm/test/Other/new-pm-thinlto-prelink-samplepgo-inline-threshold.ll:74:10: error: CHECK: expected string not found in input
; CHECK: call i32 @_Z4sum1ii
         ^

I just noticed this again when switching host compiler of the 1-stage clang-solaris-sparcv9 buildbot from llvm 15.0.0 to 16.0.0.

Should we discuss this here or is it better to file an Issue?

[huangjd]

In D143369#4292062, @ro wrote:

A reghunt just determined that this patch broke a considerable number (30+) of profile-related tests on Solaris/sparcv9. E.g.

+  LLVM :: Other/new-pm-thinlto-prelink-samplepgo-inline-threshold.ll

/vol/llvm/src/llvm-project/dist/llvm/test/Other/new-pm-thinlto-prelink-samplepgo-inline-threshold.ll:74:10: error: CHECK: expected string not found in input
; CHECK: call i32 @_Z4sum1ii
         ^

I just noticed this again when switching host compiler of the 1-stage clang-solaris-sparcv9 buildbot from llvm 15.0.0 to 16.0.0.

Should we discuss this here or is it better to file an Issue?

Could you discuss this with more detail? This patch is very simple to understand so I am surprised that it's causing such a bug

[ro]

I've taken the smallest failing testcase (LLVM :: tools/llvm-profdata/sample-remap.test) and reduced it further to

$ cat in
main:184019:0
 4: 534
 4.2: 534
$ llvm-profdata merge -sample -text in -o out

Without your patch, the output is identical to the input, with it, I get

main:184019:0
 4: 1068

instead. I'm completely unfamiliar with both the input format and the code, so I've not yet tried looking deeper.

[huangjd]

In D143369#4295219, @ro wrote:

I've taken the smallest failing testcase (LLVM :: tools/llvm-profdata/sample-remap.test) and reduced it further to

$ cat in
main:184019:0
 4: 534
 4.2: 534
$ llvm-profdata merge -sample -text in -o out

Without your patch, the output is identical to the input, with it, I get

main:184019:0
 4: 1068

instead. I'm completely unfamiliar with both the input format and the code, so I've not yet tried looking deeper.

I ran it on x86-64 linux and couldn't replicate it, so it seems like an issue specific to Solaris/sparcv9. It doesn't look like the line with 4.2 is skipped due to this patch, but instead it's parsed but being merged with the line with 4, which indicates a bug somewhere else.

[ro]

In D143369#4296667, @huangjd wrote:

What system are you running this on? I ran it on x86-64 linux and couldn't replicate it. It doesn't look like the line with 4.2 is skipped due to this patch, but instead it's parsed but being merged with the line with 4, which indicates a bug somewhere else.

This is Solaris/sparcv9 (sparcv9-sun-solaris2.11) and can also be seen on the corresponding buildbot. I'm currently running both a debug build to be able to investigate more and a Linux/sparc64 build to see if the issue occurs there, too. Unfortunately, those Debug builds are dog slow...

I've also reverted your patch on main locally: all the failures seen in the buildbot vanish then.

[huangjd]

In D143369#4296705, @ro wrote:

In D143369#4296667, @huangjd wrote:

What system are you running this on? I ran it on x86-64 linux and couldn't replicate it. It doesn't look like the line with 4.2 is skipped due to this patch, but instead it's parsed but being merged with the line with 4, which indicates a bug somewhere else.

This is Solaris/sparcv9 (sparcv9-sun-solaris2.11) and can also be seen on the corresponding buildbot. I'm currently running both a debug build to be able to investigate more and a Linux/sparc64 build to see if the issue occurs there, too. Unfortunately, those Debug builds are dog slow...

I've also reverted your patch on main locally: all the failures seen in the buildbot vanish then.

Any leads on why it is crashing with debug build?

llvm/lib/ProfileData/SampleProfReader.cpp

std::error_code SampleProfileReaderText::readImpl() {
 
   // DepthMetadata tracks whether we have processed metadata for the current
   // top-level or nested function profile.
   uint32_t DepthMetadata = 0;
 
   ProfileIsFS = ProfileIsFSDisciminator;
   FunctionSamples::ProfileIsFS = ProfileIsFS;
   for (; !LineIt.is_at_eof(); ++LineIt) {
-    if ((*LineIt)[(*LineIt).find_first_not_of(' ')] == '#')
+    size_t pos = LineIt->find_first_not_of(' ');
+    if (pos == LineIt->npos || (*LineIt)[pos] == '#')
       continue;
     // Read the header of each function.
     //
     // Note that for function identifiers we are actually expecting
     // mangled names, but we may not always get them. This happens when
     // the compiler decides not to emit the function (e.g., it was inlined
     // and removed). In this case, the binary will not have the linkage
     // name for the function, so the profiler will emit the function's

llvm/test/tools/llvm-profdata/Inputs/sample-empty-lines.proftext

+main:10:1
+ 2: 3
+ 
+
+ 3: inline1:5
+  
+  4: 1
+ 
+

llvm/test/tools/llvm-profdata/sample-empty-lines.test

+Test llvm-profdata merge can handle empty line with spaces in text format sample profile.
+
+RUN: llvm-profdata merge --sample --text %p/Inputs/sample-empty-lines.proftext | FileCheck %s
+CHECK: main:10:1
+CHECK-NEXT: 2: 3
+CHECK-NEXT: 3: inline1:5
+CHECK-NEXT: 4: 1
+