[PATCH] D44847: [profile] Fix value profile runtime merging issues

[Rong Xu via Phabricator via llvm-commits]

xur added inline comments.


================
Comment at: lib/profile/InstrProfilingFile.c:230
+  // (which can leave garbage at the end of profile file).
+  if (!VPMergeHook)
+    truncate(ProfileFileName, 0);
----------------
davidxl wrote:
> xur wrote:
> > davidxl wrote:
> > > xur wrote:
> > > > davidxl wrote:
> > > > > non value profiling data has fixed size, so why is this needed?
> > > > Because there are still value counts in both in-memory and in-disk profiles.
> > > > 
> > > > Using the test in the patch as the example (running on my machine).
> > > > assuming the binary is named as 'gen' and VPMergeHook null.
> > > > 
> > > > if we don't do online merge,
> > > > "gen" will generate a profile of size 320 bytes.
> > > > "gen 1" will generate a profile with size of 352 bytes
> > > > 
> > > > If we run "gen 1 && gen", the result binary is still 352 bytes.
> > > > But the effective size is 320 bytes. The tail 32 bytes are trash.
> > > > 
> > > > 
> > > Without VpMergeHook, we should assert that there is no value profile data -- it should not be dumped in the first place.
> > current behavior is to write out the in-memory value profile data and discard the value profile data in the file. if there is not value profile from the file, the result is still correct.
> > 
> > I'm not familiar with the use cast the not having libc (where VPMergeHook will be nullptr). Is it not suppose to have any value profile?
> > 
> > I can assert VPMergeHook==nullptr and in-memory valuesites are not empty. That will be easy. Is this what you suggested?
> In lprofGetVPDataReader method, returns null if VPMergeHook is null.
This will generate incorrect profile: the valuesite in the data section says there are valuesites but VP count is empty.

if we do this, the behavior would be: llvm-profdata show or merge command will stop silently at the first function that has the value-site instrumentation. The rest profile will be discarded. 

Note, of course, this only applies to  VPMergeHook==nullptr case.


https://reviews.llvm.org/D44847