Page MenuHomePhabricator

[InstrProf][NFC] Do not assume size of counter type
ClosedPublic

Authored by ellis on Wed, Dec 22, 11:26 AM.

Details

Summary

Existing code tended to assume that counters had type uint64_t and
computed size from the number of counters. Fix this code to directly
compute the counters size in number of bytes where possible. When the
number of counters is needed, use __llvm_profile_counter_entry_size()
or getCounterTypeSize(). In a later diff these functions will depend
on the profile mode.

Change the meaning of DataSize and CountersSize to make them more clear.

  • DataSize (CountersSize) - the size of the data (counter) section in bytes.
  • NumData (NumCounters) - the number of data (counter) entries.

Diff Detail

Event Timeline

ellis created this revision.Wed, Dec 22, 11:26 AM
ellis published this revision for review.Wed, Dec 22, 11:30 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptWed, Dec 22, 11:30 AM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript
kyulee added inline comments.Tue, Dec 28, 3:30 PM
compiler-rt/lib/profile/InstrProfilingMerge.c
25–26

CounterSize means NumCounters similar to DataSize which should mean NumData.
You rename this for Counter not for Data. But down the road below, Header still uses CounterSize and DataSize.
Should we keep CounterSize same as before?

156

SrcCounters + NC may be differently evaluated in between uint64_t *SrcCounters vs char *SrcCounters.

llvm/lib/ProfileData/InstrProfReader.cpp
463

CountersDelta should be 0 when being read in the header for the Correlator?
Do we keep this as an invariant? I guess advance() above in the main loop may not update this value for this case.

469

It's a bit confusing !Correlator is repeated in here and below conditions.
BTW, should it be CounterBaseOffset >= MaxCounterOffset? I think MaxCounterOffset is not really a valid offset for a counter since it's just the starting offset of the name section that follows.

478

I think the same here. LastCounterBaseOffset >= MaxCounterOffset?
And also, looking at these two conditions, the latter condition (checking the end address) seems sufficient instead of checking if both the start and end address are out of bound.

Given this change must be NFC, but subtle due to pointer arithmetic. I guess you might run more through tests with some large programs with PGO.

ellis updated this revision to Diff 396594.Wed, Dec 29, 7:19 PM
ellis marked 5 inline comments as done.

Change the meaning of DataSize to mean the size of the data section in bytes rather than the number of data entries.

ellis added a comment.EditedWed, Dec 29, 7:19 PM

Given this change must be NFC, but subtle due to pointer arithmetic. I guess you might run more through tests with some large programs with PGO.

Thanks for the review! Instead of running tests on larger programs, I think it's more important to run tests on different modes and platforms to exercise compiler-rt.

compiler-rt/lib/profile/InstrProfilingMerge.c
25–26

I've decided to go ahead change the diff so that Num{Counters,Data} and {Counters,Data}Size have the expected meaning.

73

By the way, this change has different behavior because it's actually a bug fix. Header->CounterSize (which is the number of counters) needs to be scaled by the entry size.

156

Thanks for the catch!

llvm/lib/ProfileData/InstrProfReader.cpp
463

CountersDelta is now a constant zero when there is a Correlator now that I've changed advanceData().

469

Thanks, I made a few fixes to this area. I'm now tracking CountersEnd so that we can correctly compute MaxNumCounters and we no longer care if we have a correlator. Also, the two checks are helpful to make sure CountersEnd - (CountersStart + CounterBaseOffset) doesn't underflow

ellis edited the summary of this revision. (Show Details)Wed, Dec 29, 7:22 PM
ellis updated this revision to Diff 396598.Wed, Dec 29, 8:21 PM

Fix cast warning.

kyulee added inline comments.Thu, Dec 30, 10:06 AM
compiler-rt/lib/profile/InstrProfilingMerge.c
159

This also assumes the counter type is a 64 bit integer. Should we just use the counter element size like NC * __llvm_profile_counter_entry_size() for the loop bound?
It appears this is actually just a memcpy, and I guess the compiler will emit it for you.

llvm/include/llvm/ProfileData/InstrProfReader.h
317

counts -> counter

llvm/test/tools/llvm-profdata/malformed-ptr-to-counter-array.test
6

DataSize -> NumData
Same thing for Counter.

56

Why is the behavior changed?
My understanding is that now you check # of counters against the maximum # of counters read from the header which appears 0.
What happens if we fix it in the header as 2 so that # of counters are okay because of type size truncation, but still off by 1?

ellis updated this revision to Diff 396708.Thu, Dec 30, 12:46 PM
ellis marked an inline comment as done.

Fix reader error checking code.

ellis marked an inline comment as done.Thu, Dec 30, 12:49 PM
ellis added inline comments.
compiler-rt/lib/profile/InstrProfilingMerge.c
159

Yes it does assume 64 bits, but that is the only mode when this diff lands. In the next diff the behavior changes based on the mode.

Since these values are being accumulated, I'm not sure it could be just a memcpy?

llvm/test/tools/llvm-profdata/malformed-ptr-to-counter-array.test
21

This is the CountersDelta value and it was too small for the comment below to be true. Set the least significant byte to 8 to make CounterOffset = 1.

56

The error message in the original code is actually talking about the total number of counters. There are two total counters but the reader expected too read a third counter.

The new message is talking about number of counters for a function. This function has two counters, but based on the offset there can only be zero more counters. On second thought this is quite confusing....

For this latest revision, there are really only two things to check for a function's counter offset.

  1. Make sure it is positive
  2. Make sure this function's last counter is inside the counters section

I've updated the error checking code to reflect this. Let me know what you think and sorry for so many revisions on this part.

kyulee accepted this revision.Thu, Dec 30, 2:26 PM

LGTM

compiler-rt/lib/profile/InstrProfilingMerge.c
159

Ah. I thought it's a copy not an accumulation. Given the current counter type is 64 bit only, this looks okay.

llvm/test/tools/llvm-profdata/malformed-ptr-to-counter-array.test
56

Yeah. I think in your initial diff I thought/commented this way -- either the start is positive or the last offset is within the the section.

This revision is now accepted and ready to land.Thu, Dec 30, 2:26 PM
ellis updated this revision to Diff 399830.Thu, Jan 13, 4:29 PM

Rebase and test

This revision was automatically updated to reflect the committed changes.

It looks like this (expectedly) broke our downstream builder (https://ci.chromium.org/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-debug/b8824668235249206913/overview). We can do a soft transition by checking against the clang git revision in our build, then setting a macro if the clang includes this change, then toggling between types based on the macro, but this seems pretty messy. For future commits, would this perhaps constitute an update to INSTR_PROF_RAW_VERSION? If that's mainly reserved for binary format changes, would it perhaps be a good idea to introduce a macro for source-level API changes? While the binary format hasn't changed, downstream projects that use InstrProfData.inc could break from stuff like this.

ellis added a comment.Tue, Jan 18, 4:58 PM

It looks like this (expectedly) broke our downstream builder (https://ci.chromium.org/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-debug/b8824668235249206913/overview). We can do a soft transition by checking against the clang git revision in our build, then setting a macro if the clang includes this change, then toggling between types based on the macro, but this seems pretty messy. For future commits, would this perhaps constitute an update to INSTR_PROF_RAW_VERSION? If that's mainly reserved for binary format changes, would it perhaps be a good idea to introduce a macro for source-level API changes? While the binary format hasn't changed, downstream projects that use InstrProfData.inc could break from stuff like this.

Hi @leonardchan, apologies for breaking your builder. Can you help me understand why changing InstrProfData.inc resulted in a build break? Does this mean any change to that file will break your builds?

The changes to InstrProfData.inc in this patch are pretty minimal (they are just renaming variables) and could easily be reverted. I'd rather to this than bumping the version.

phosek added a subscriber: phosek.Tue, Jan 18, 4:59 PM

Is it really necessary to rename the fields in InstrProfData.inc? That header is shipped with Clang and may have other uses outside of compiler-rt. For example, in Fuchsia we use InstrProfData.inc in the kernel where we cannot use the existing runtime and the name change broke our build. The original names may not be perfect, but that shouldn't justify breaking source compatibility.

Is it really necessary to rename the fields in InstrProfData.inc? That header is shipped with Clang and may have other uses outside of compiler-rt. For example, in Fuchsia we use InstrProfData.inc in the kernel where we cannot use the existing runtime and the name change broke our build. The original names may not be perfect, but that shouldn't justify breaking source compatibility.

(I tend to think that changing the name is fine since the type is changed and it serves as a hint that downstream needs to adjust their APIs, not causing lurking problems. By I am happy that my opinion is overridden by others who have strong opinions.)

(I'd probably not call the change NFC since APIs have been renamed.)

This is the kind of changes I hope that past coverage contributors like @davidxl, @phosek, @vsk can comment before landing...

ellis added a comment.Tue, Jan 18, 5:41 PM

Is it really necessary to rename the fields in InstrProfData.inc? That header is shipped with Clang and may have other uses outside of compiler-rt. For example, in Fuchsia we use InstrProfData.inc in the kernel where we cannot use the existing runtime and the name change broke our build. The original names may not be perfect, but that shouldn't justify breaking source compatibility.

I've created D117631 to restore InstrProfData.inc and hopefully fix your build. In retrospect I guess this wasn't NFC because this file is being used elsewhere, good to know.

By the way, I'm also changing this file in D116180, but I'm guessing that won't cause issues because it is only adding a bit field, not changing a field name.

Please understand that InstrProfData.inc is now a public installed header file in the compiler-rt installation, at <profile/InstrProfData.inc>. So *any* changes to this file have a quite high standard of compatibility concern.

Is it really necessary to rename the fields in InstrProfData.inc? That header is shipped with Clang and may have other uses outside of compiler-rt. For example, in Fuchsia we use InstrProfData.inc in the kernel where we cannot use the existing runtime and the name change broke our build. The original names may not be perfect, but that shouldn't justify breaking source compatibility.

I've created D117631 to restore InstrProfData.inc and hopefully fix your build. In retrospect I guess this wasn't NFC because this file is being used elsewhere, good to know.

Thanks!

By the way, I'm also changing this file in D116180, but I'm guessing that won't cause issues because it is only adding a bit field, not changing a field name.

I looked at that change and I don't think it should introduce any issues but thanks for checking.

gulfem added a subscriber: gulfem.Tue, Jan 18, 6:30 PM