[PATCH] D98135: [OpenMP][InstrProfiling] Fix a missing instr profiling counter

[Xun Li via Phabricator via cfe-commits]

lxfind added a comment.

In D98135#2650940 <https://reviews.llvm.org/D98135#2650940>, @ABataev wrote:

> In D98135#2650914 <https://reviews.llvm.org/D98135#2650914>, @lxfind wrote:
>
>> @ABataev, wondering if you have a timeline on this?
>> Missing counters from OMP functions sometimes cause llvm-cov to crash because of data inconsistency.
>
> Cannot answer right now. It would be much easier to fix this if you could provide additional info about what tests are exactly failed, what are the constructs that do not support it, etc.

Yes the whole pipeline is a bit long and complex, so I don't have an exact repro in hand because it requires source code and run it.

But let me try to explain what happened in my observation. There are two sections that are related to this issue in the binary, the IPSK_covfun section that contains the function records, and the IPSK_name section that contains the list of all function names. The issue here is that some OMP functions that are found in the IPSK_covfun section are not found in the IPSK_name section.

The records in IPSK_covfun are generated like this:

Whenever CodeGenFunction is generating code for any function, it will first call the `CodeGenFunction::GenerateCode()` function, in which it will call `PGO.assignRegionCounters(GD, CurFn);`: https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/clang/lib/CodeGen/CodeGenFunction.cpp#L1329

>From there, it will call `emitCounterRegionMapping`:
https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/clang/lib/CodeGen/CodeGenPGO.cpp#L819

which will then call: `CGM.getCoverageMapping()->addFunctionMappingRecord`:
https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/clang/lib/CodeGen/CodeGenPGO.cpp#L890

which will eventually add this function to a `FunctionRecords`:
https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/clang/lib/CodeGen/CoverageMappingGen.cpp#L1655

So all above is to show that every function will eventually be added to `FunctionRecords`, unless in the case where a function is explicitly marked as unused. The `FunctionRecords` will eventually be all written into the `IPSK_covfun` section in the binary.

The names in IPSK_name section are generated like this:
Within InstrProfiling.cpp (https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp), it collects all the function names referenced in all instrumentation counter increments instructions:
https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp#L920
Basically for every InstrProfIncrementInst, it adds the function name referenced in it to a list `ReferencedNames`.
Then in the end this `ReferencedNames` is written into the IPSK_name section.

Now you can see that for the OMP functions  that don't have any counters, they are still added to `FunctionRecords`, but not added to `ReferencedNames`, because they are not referenced by any InstrProfIncrementInst.
During the running of llvm-cov, when reading the list of function records, it will attempt to look up the name of the function from the function name list:
https://github.com/llvm/llvm-project/blob/b9ff67099ad6da931976e66f1510c5af2558a86e/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp#L560

And it will not be able to find it for the OMP case, so it will return an error.

Overall this is very complex and a bit fragile to me. For instance, we probably could have detected the error much earlier during Instrumentation pass in LLVM, that some function records' names are not in the name list. Or we could simply construct the list of function names based on the function records. But currently these two are generated independently.
cc @MaskRay and @vsk, maybe they have thoughts on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98135