Page MenuHomePhabricator

[profile] Add binary id into profiles
ClosedPublic

Authored by gulfem on May 6 2021, 6:01 PM.

Details

Summary

This patch adds binary id into profiles to easily associate binaries
with the corresponding profiles. There is also an RFC that
discusses the motivation, design and implementation in more detail:
https://lists.llvm.org/pipermail/llvm-dev/2021-June/151154.html

Diff Detail

Unit TestsFailed

TimeTest
20 msx64 debian > LLVM.tools/llvm-profdata::c-general.test
Script: -- : 'RUN: at line 11'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-profdata show /mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-profdata/Inputs/c-general.profraw -o - | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-profdata/c-general.test
30 msx64 debian > LLVM.tools/llvm-profdata::malformed-ptr-to-counter-array.test
Script: -- : 'RUN: at line 12'; printf '\201rforpl\377' > /mnt/disks/ssd0/agent/llvm-project/build/test/tools/llvm-profdata/Output/malformed-ptr-to-counter-array.test.tmp.profraw
30 msx64 debian > LLVM.tools/llvm-profdata::raw-32-bits-be.test
Script: -- : 'RUN: at line 1'; printf '\377lprofR\201' > /mnt/disks/ssd0/agent/llvm-project/build/test/tools/llvm-profdata/Output/raw-32-bits-be.test.tmp
40 msx64 debian > LLVM.tools/llvm-profdata::raw-32-bits-le.test
Script: -- : 'RUN: at line 1'; printf '\201Rforpl\377' > /mnt/disks/ssd0/agent/llvm-project/build/test/tools/llvm-profdata/Output/raw-32-bits-le.test.tmp
30 msx64 debian > LLVM.tools/llvm-profdata::raw-64-bits-be.test
Script: -- : 'RUN: at line 1'; printf '\377lprofr\201' > /mnt/disks/ssd0/agent/llvm-project/build/test/tools/llvm-profdata/Output/raw-64-bits-be.test.tmp
View Full Test Results (15 Failed)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
gulfem edited the summary of this revision. (Show Details)May 11 2021, 10:38 AM
gulfem marked an inline comment as done.May 11 2021, 10:46 AM
gulfem added inline comments.
compiler-rt/include/profile/InstrProfData.inc
132

Can a raw profile with build id and another raw profile without a build id be merged and create indexed profiled?

compiler-rt/lib/profile/InstrProfilingWriter.c
36

I renamed it to binary id, and I will briefly explain other ids used in other platforms in the RFC.

291

It seems like ValueProfiling already uses heap allocation compiler-rt/lib/profile/InstrProfilingValue.c, but we don't really need heap allocation for build id.
So, I removed build id struct.
Please let me know what you think about the new implementation.

gulfem added inline comments.May 12 2021, 6:18 PM
compiler-rt/include/profile/InstrProfData.inc
132

@phosek, it seems to me like raw and indexed profile do not share the same profile format.
For ex, raw profile header is defined in:
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ProfileData/InstrProfData.inc#L130

Whereas, indexed profile header is defined in:
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ProfileData/InstrProf.h#L999

I think, we might need to extend both formats with binary id then.
Please let me know if I'm missing anything.

phosek added inline comments.May 12 2021, 11:33 PM
compiler-rt/include/profile/InstrProfData.inc
132

Yes, looks like we'll need to extend both formats but I still think it'd be preferable to support arbitrary number of binary id's in both formats since at least in ELF, you can have more than one build ID. The format of should also be the same in both the raw and indexed format, that is a sequence of tuples where each tuple is a length and a sequence of bytes of that length.

compiler-rt/lib/profile/InstrProfiling.h
101

I'd slightly prefer returning both the data and the size as output parameters for consistency. We might also consider having a boolean/int return argument to signal an error.

compiler-rt/lib/profile/InstrProfilingWriter.c
287–288

You shouldn't need an extra variable for the pointer.

291

That's true but it's also possible to allocate counter for value profiling statically if you use -mllvm -vp-static-alloc in which case you can avoid malloc which may be desirable on some platforms (this is what we would likely use on Fuchsia for example).

llvm/tools/llvm-profdata/llvm-profdata.cpp
2227

I think we'll want this functionality even in the final version but we should introduce additional flag, for example -show-binary-id, and only print the binary id if that flag is set.

gulfem updated this revision to Diff 348352.May 27 2021, 12:14 PM
  1. Support multiple binary ids
  2. Extend indexed profile format to include binary ids
  3. Add tests
gulfem updated this revision to Diff 348420.May 27 2021, 5:09 PM

Use clang-format to correctly format the code

mcgrathr added inline comments.Jun 1 2021, 6:26 PM
compiler-rt/lib/profile/InstrProfilingPlatformLinux.c
100

This is insufficient. The note "name" determines the space of n_type values. You must verify that the n_namesz=4 and the name bytes are "GNU\0" as well, or else this is some unrelated note that happens to use n_type=3 where 3 does not mean NT_GNU_BUILD_ID.

103

This arithmetic is not right since Note has a type with sizeof>1. An example of correct arithmetic is:

Note = (const ElfW(Nhdr)*)((const char *)(Note + 1) + RoundUp(Note->n_namesz, 4) + RoundUp(Note->n_descsz, 4));
118

Can you abstract this logic into a subroutine so the code is not repeated in the two functions?
A simple approach would be a subroutine that takes a maybe-null ProfDataWriter pointer and when called with a null pointer just returns the size instead of calling the writer.

compiler-rt/test/profile/binary-id.c
1 ↗(On Diff #348420)

You may need to pass explicit -Wl,--build-id=none through the compiler here since it's sometimes on by default.

llvm/include/llvm/ProfileData/InstrProfReader.h
85

Is this the number of distinct IDs, or the byte size of the total block? (AIUI the total block is a sequence of {uint64_t n, uint8_t[n]} blocks.) If it's the number of distinct IDs, then I don't understand how a single uint8_t* from getBinaryIds() is meant to be used. For a number like that, I think Count is a clearer name to use than Size.

91

Should these really be two separate methods? Or should it just be one method that returns both the size and the data in a span-style data structure?

491

I don't understand the commented-out declaration.

llvm/lib/ProfileData/InstrProfReader.cpp
542

This is wildly susceptible to bad data.

gulfem updated this revision to Diff 349985.Jun 4 2021, 3:59 PM

Addressed Roland's comments

gulfem marked 4 inline comments as done.Jun 4 2021, 4:14 PM
gulfem added inline comments.
compiler-rt/lib/profile/InstrProfilingPlatformLinux.c
103

My understanding is that each note in notes section starts with a struct which includes n_namesz, n_descsz, and n_type members.
It is followed by the name (whose length is defined in n_namesz) and then by the descriptor (whose length is defined in n_descsz).
So, type is already part of the struct. The arithmetic I have increases Note by struct size, and name data, and descriptor data.
Am I missing anything?

llvm/include/llvm/ProfileData/InstrProfReader.h
85

Is this the number of distinct IDs, or the byte size of the total block?

It is the number of binary IDs. That file consistently uses Size to refer to number of elements like DataSize, CounterSize, NamesSize, etc.
I don't like that either, but I just used Size to be consistent with the rest of the implementation.
I think Count or NumOf is more self-explanatory.

llvm/lib/ProfileData/InstrProfReader.cpp
542

This is wildly susceptible to bad data.

542

How can I improve that? I need to somehow increment the pointer by binary id length.

gulfem marked an inline comment as done.Jun 4 2021, 4:22 PM
phosek added inline comments.Jun 14 2021, 1:25 AM
compiler-rt/include/profile/InstrProfData.inc
132

I'd prefer to put this field at the end of the header to match the position of binary ID in the profile.

compiler-rt/lib/profile/InstrProfilingInternal.h
206 ↗(On Diff #349985)

This name is a bit of a mouthful. I'd consider calling it just __llvm_write_binary_ids and say in the comment that the function always returns the number of binary ids even if writer is NULL.

compiler-rt/lib/profile/InstrProfilingPlatformLinux.c
103

I think we should be returning the number of binary ids in every case, even if writer != NULL.

103

When you increment a pointer, the address is moved by the size of the underlying type:

char* c = ...;
c++; // c increments by 1 == sizeof(char)
uint64_t* l = ...;
l++; // l increments by 8 == sizeof(uint64_t)

In this case sizeof(Note) > 1 so you first need to cast it to const char *.

llvm/lib/ProfileData/InstrProfReader.cpp
518

Where is this deallocated? It seems like this memory gets leaked. BinaryIdTy is just a plain struct with just two 8 byte fields (on a 64-bit machine), can we avoid allocating it on the heap and return/pass it by value to simplify lifetime management?

542

I think at minimum, you need to check that you aren't reading past the end of the file in the case the length is incorrect.

gulfem updated this revision to Diff 352576.Jun 16 2021, 4:01 PM

Addressed Petr's comments

gulfem marked 4 inline comments as done.Jun 16 2021, 4:06 PM
gulfem added inline comments.
llvm/lib/ProfileData/InstrProfReader.cpp
518

I used unique_ptr, so memory will be released after BinaryIdTy goes out of scope.

gulfem marked 2 inline comments as done.Jun 16 2021, 4:07 PM
gulfem edited the summary of this revision. (Show Details)Jun 16 2021, 4:11 PM
gulfem added reviewers: vsk, davidxl.
phosek added inline comments.Jun 16 2021, 4:12 PM
llvm/lib/ProfileData/InstrProfReader.cpp
518

I'd still prefer passing these by value, heap allocation in this case seems unnecessary and is going to be less efficient.

gulfem retitled this revision from [profile] WIP Add binary id into profiles to [profile] Add binary id into profiles.Jun 24 2021, 2:28 PM
gulfem edited the summary of this revision. (Show Details)
gulfem edited the summary of this revision. (Show Details)Jun 24 2021, 2:32 PM

@vsk @davidxl Do you have any thoughts on this? We've already done a few rounds of reviews, but we'd like to get your opinion as well.

Repeating the same question asked in RFC: the profile data has builtin fine grained matching mechanism. For the top level stamping, is it enough to embed the build id in the file name of the profile data?

Repeating the same question asked in RFC: the profile data has builtin fine grained matching mechanism. For the top level stamping, is it enough to embed the build id in the file name of the profile data?

Just to clarify: we are suggesting to embed build in the profile file itself, not in the profile file name.
This will also allow us to embed multiple build ids if profiles are merged together.
Can you please elaborate on the builtin fine grained matching mechanism and why do you think it will be a problem?

Repeating the same question asked in RFC: the profile data has builtin fine grained matching mechanism. For the top level stamping, is it enough to embed the build id in the file name of the profile data?

We need to be able to match the profile back to the corresponding binary when generating coverage. Embedding build ID in filename doesn't work for us because on Fuchsia, most processes don't have access to filesystem (it's a capability that most processes shouldn't need). Rather the profile is exported via IPC as a memory object that doesn't have any name.

There might be a Fuchsia-specific solution, like wrapping the profile in an envelope that contains he build ID, but that solution would be Fuchsia-specific and we thought that making profiles more self-descriptive could be generally useful.

One idea we would like to pursue in the future is implementing support for the debuginfod protocol in LLVM, including llvm-cov, you could then use llvm-cov --instr-profile default.profraw and llvm-cov would fetch both the binary and the source from debuginfod server using the build ID inside the profile. In our case, this would not only simplify our infrastructure but also make it easy for developers to generate coverage reports from profiles fetched directly from our CI to reproduce results.

I am less concerned about raw profile format change (the version still needs to be bumped), but for the indexed format, the version needs to be bumped and it needs to guarantee that format is backward compatible. The patch does not seem to handle it.

Is it necessary to have binary id support in the indexed format?

I am less concerned about raw profile format change (the version still needs to be bumped), but for the indexed format, the version needs to be bumped and it needs to guarantee that format is backward compatible. The patch does not seem to handle it.

Is it necessary to have binary id support in the indexed format?

Yes, we need to bump the version. There is still some work that needs to be done in this patch like bumping the version, adjusting some of the tests, and adding more tests, etc.
We would like to get early feedback to see whether there is any fundamental issue about the approach.
@phosek can correct me, but I think we need to have binary id support in the indexed format.
We merge raw profiles into indexed profiles before generating source code coverage reports, and in order to associate binaries while generating coverage, we need to have binary id in the indexed profiles.

I am less concerned about raw profile format change (the version still needs to be bumped), but for the indexed format, the version needs to be bumped and it needs to guarantee that format is backward compatible. The patch does not seem to handle it.

Is it necessary to have binary id support in the indexed format?

Yes, we need to bump the version. There is still some work that needs to be done in this patch like bumping the version, adjusting some of the tests, and adding more tests, etc.
We would like to get early feedback to see whether there is any fundamental issue about the approach.
@phosek can correct me, but I think we need to have binary id support in the indexed format.
We merge raw profiles into indexed profiles before generating source code coverage reports, and in order to associate binaries while generating coverage, we need to have binary id in the indexed profiles.

Is the merging done in process or offline? Assuming it is offline, it seems possible to use name base approach.

Is the merging done in process or offline? Assuming it is offline, it seems possible to use name base approach.

It's done offline but since the profiles published via IPC don't have any names, we cannot rely or filenames, we would need to introduce a custom Fuchsia wrapper format (see my previous comment about the "envelope" idea).

We don't necessarily need binary ID in the indexed format, what we could do is:

llvm-profdata show --binary-id a.profraw >a.buildid
# fetch a binary with a.buildid as a.out
llvm-profdata show --binary-id b.profraw >b.buildid
# fetch a binary with b.buildid as b.out
...
llvm-profdata merge -o merged.profdata a.profraw b.profraw ...
llvm-cov show --instr-profile=merged.profdata a.out b.out ...

If we also stored binary IDs inside the indexed format, we could simplify this and do:

llvm-profdata show --binary-ids merged.profdata >merged.buildids
# fetch all binaries

This is more efficient but either would be fine with us for now.

Storing binary IDs inside the indexed profile would become really valuable if/once we have support debuginfod support at which point we should be able to just do:

llvm-profdata merge -o merged.profdata a.profraw b.profraw ...
llvm-cov show --instr-profile=merged.profdata

and llvm-cov would fetch binaries directly from the debuginfod server using binary IDs stored inside the indexed profile. It'll take a while before we have debuginfod support available in LLVM though so this is not critical for now, we're just trying to plan ahead.

Is extending the indexed format a problem? Is there some way to make it less of an issue?

Is the merging done in process or offline? Assuming it is offline, it seems possible to use name base approach.

It's done offline but since the profiles published via IPC don't have any names, we cannot rely or filenames, we would need to introduce a custom Fuchsia wrapper format (see my previous comment about the "envelope" idea).

We don't necessarily need binary ID in the indexed format, what we could do is:

llvm-profdata show --binary-id a.profraw >a.buildid
# fetch a binary with a.buildid as a.out
llvm-profdata show --binary-id b.profraw >b.buildid
# fetch a binary with b.buildid as b.out
...
llvm-profdata merge -o merged.profdata a.profraw b.profraw ...
llvm-cov show --instr-profile=merged.profdata a.out b.out ...

If we also stored binary IDs inside the indexed format, we could simplify this and do:

llvm-profdata show --binary-ids merged.profdata >merged.buildids
# fetch all binaries

This is more efficient but either would be fine with us for now.

Storing binary IDs inside the indexed profile would become really valuable if/once we have support debuginfod support at which point we should be able to just do:

llvm-profdata merge -o merged.profdata a.profraw b.profraw ...
llvm-cov show --instr-profile=merged.profdata

and llvm-cov would fetch binaries directly from the debuginfod server using binary IDs stored inside the indexed profile. It'll take a while before we have debuginfod support available in LLVM though so this is not critical for now, we're just trying to plan ahead.

I suppose this workflow can also be done with

llvm-cov show --instr-profile=merged.profdata --buildids=<file of list of ids> without the need to fetch the binaries.

Is extending the indexed format a problem? Is there some way to make it less of an issue?

It is not an issue, but I think it is preferable minimize version bumps for indexed format to reduce longer term churns. Given this restriction, it is better to make version change tied to functionalities that can do without a format change. Scripts, wrappers etc. are better for non-essential changes -- especially for workflows that can be mostly automated (which minimizes human inconveniences).

gulfem updated this revision to Diff 359056.Jul 15 2021, 11:04 AM
  1. Add binary id between profile header and data
  2. Raw profile version bump
  3. Only add binary id into raw profiles
aeubanks resigned from this revision.Jul 15 2021, 11:15 AM

I'm not familiar enough with this code

I'm not familiar enough with this code

@aeubanks I just added you because I modified a test file (corrupted-profile.c), and you are the author of that test file.

that test change seems fine
I remember I copied the structure from another existing test though, somewhere in llvm/test/tools/llvm-profdata IIRC

that test change seems fine
I remember I copied the structure from another existing test though, somewhere in llvm/test/tools/llvm-profdata IIRC

Thanks!

@davidxl we split the indexed profile change out of this change since it's not necessary for us right now, would it be possible to take a look again?

compiler-rt/include/profile/InstrProfData.inc
141

I'd prefer to move this field just after Version before DataSize to match the order of data inside the profile.

davidxl added inline comments.Jul 19 2021, 1:08 PM
compiler-rt/lib/profile/InstrProfilingPlatformLinux.c
90

Is it possible to extract this into a common helper function like

void ForEachNote(int note_type, .. note_start, .. note_end, ... call_back ) {

...

}

gulfem updated this revision to Diff 359943.Jul 19 2021, 3:40 PM
  1. Break WriteBinaryIds into multiple functions to increase readibility
  2. Rebase
gulfem marked an inline comment as done.Jul 19 2021, 3:43 PM
gulfem added inline comments.
compiler-rt/lib/profile/InstrProfilingPlatformLinux.c
90

I think your concern was about the readability of that function, so I broke that function into multiple functions to increase readability.
Please let me know if you other concerns.

davidxl accepted this revision.Jul 19 2021, 3:51 PM

LGTM

llvm/lib/ProfileData/InstrProfReader.cpp
538

When there is no binary ids to be print, perhaps output a line with 'None' instead of empty (which looks like the output is truncated.

This revision is now accepted and ready to land.Jul 19 2021, 3:51 PM
gulfem marked an inline comment as done.Jul 19 2021, 4:12 PM
gulfem added inline comments.
llvm/lib/ProfileData/InstrProfReader.cpp
538

When there is no binary id, we don't print anything at all.
At line 517, we check the binary id size, and if there is no binary id, just return without printing anything.

if (BinaryIdsSize == 0)
  return success();
gulfem updated this revision to Diff 360315.Jul 20 2021, 4:29 PM

Modified some raw profile test files to include the new profile version (version 6) and new header element (BinaryIdsSize).

This revision was landed with ongoing or failed builds.Jul 21 2021, 10:56 AM
This revision was automatically updated to reflect the committed changes.