[PGO] fix bogus warning for merging empty llvm profile file

[xur]

Summary

Profile runtime can generate an empty profile (when there is no function in the shared library). This empty profile is treated as a text format profile. A test format profile without the flag of "#IR" is thought to be a clang generated profile. So in llvm profile merging, we will get a bogus warning of "Merge IR generated profile with Clang generated profile."

The fix here is to skip the empty profile (when the buffer size is 0) by returning nullptr in InstrProfReader::create().

This patch also fixes a use-after-move instance in loadInput() in llvm-profdata.cpp.

[davidxl]

A test case is needed.

tools/llvm-profdata/llvm-profdata.cpp

159

unrelated change?

[xur]

tools/llvm-profdata/llvm-profdata.cpp

159

this is for the use-after-move fix I mentioned in the description.
You want to split to another patch?

[vsk]

tools/llvm-profdata/llvm-profdata.cpp

159

I think it'd be better to land this change separately.

[xur]

Integrated the review comments from David and Vedant:
(1) add a test,
(2) split the use-after-move fix to another patch.

Thanks!

-Rong

[davidxl]

test/tools/llvm-profdata/merge_empty_profile.test

3 ↗

(On Diff #75028)

there is no need to check in an empty file. Just 'touch %t_empty.proftext' or 'echo -n "" > %t_empty.proftext"

[vsk]

Thanks for splitting the patch up. Please upload diffs with more context in the future (e.g git diff -U1000).

include/llvm/ProfileData/InstrProfReader.h

108

This breaks CoverageMapping::load(), and creates an inconsistency between the API's for InstrProfReader::create and IndexedInstrProfReader::create (one can successfully return nullptr, the other can't).

I'd prefer for this functionality to be implemented via a new instrprof_error. Clients can opt into handling this as a 'soft'/recoverable error, or fail with an appropriate diagnostic packaged up by the Error.

tools/llvm-profdata/llvm-profdata.cpp

149

In llvm-profdata's loadInput, you could write:

if ((Error E = ReaderOrErr.takeError())) {
  if (E != instrprof_error::empty_profile)
    WC->Err = std::move(E);
  return;
}

[xur]

Followed vsk's suggestion to use a new error code for empty file case.
Note that this does not apply to indexed profile where we don't expected empty file (in which case, we will report a format error).

Also changed to use 'touch' rather than the empty profile in Inputs (suggested by david).

-Rong

[vsk]

Thanks! lgtm with a nit.

lib/ProfileData/InstrProf.cpp

74 ↗

(On Diff #75233)

Could you add a space between raw and profile?

include/llvm/ProfileData/InstrProfReader.h

   }
 
   /// Factory method to create an appropriately typed reader for the given
-  /// instrprof file.
+  /// instrprof file. Nullptr will be returned for an empty profile file.
   static Expected<std::unique_ptr<InstrProfReader>> create(const Twine &Path);
 
   static Expected<std::unique_ptr<InstrProfReader>>

lib/ProfileData/InstrProfReader.cpp

   if (Buffer->getBufferSize() > std::numeric_limits<unsigned>::max())
     return make_error<InstrProfError>(instrprof_error::too_large);
 
+  // Return nullptr for an empty file.
+  if (Buffer->getBufferSize() == 0)
+    return std::unique_ptr<InstrProfReader>{};
+
   std::unique_ptr<InstrProfReader> Result;
   // Create the reader.
   if (IndexedInstrProfReader::hasFormat(*Buffer))

tools/llvm-profdata/llvm-profdata.cpp

     return;
 
   auto Reader = std::move(ReaderOrErr.get());
+  // Nullptr Reader means the file is empty. Skip it.
+  if (!Reader)
+    return;
   bool IsIRProfile = Reader->isIRLevelProfile();
   if (WC->Writer.setIsIRLevelProfile(IsIRProfile)) {
     WC->Err = make_error<StringError>(

   }
 
   for (auto &I : *Reader) {
+    const StringRef FuncName = I.Name;
     if (Error E = WC->Writer.addRecord(std::move(I), Input.Weight)) {
       // Only show hint the first time an error occurs.
       instrprof_error IPE = InstrProfError::take(std::move(E));

       std::unique_lock<std::mutex> ErrGuard{WC->ErrLock};
       bool firstTime = WC->WriterErrorCodes.insert(IPE).second;
       handleMergeWriterError(make_error<InstrProfError>(IPE), Input.Filename,
-                             I.Name, firstTime);
+                             FuncName, firstTime);
     }
   }
   if (Reader->hasError())