[compiler-rt][profile] Make corrupted-profile.c more robuset

[leonardchan]

Summary

This test specifically checks that profiles are not mergeable if there's a change in the CounterPtr in the profile header. The test manually changes CounterPtr by explicitly calling memset on some offset into the profile file. This test would fail if binary IDs were emitted because the offset calculation does not take into account the binary ID sizes.

This patch updates the test to use types provided in profile/InstrProfData.inc to make it more resistant to profile layout changes.

[aeubanks]

does not having the build id change the header size?

[leonardchan]

In D110277#3016205, @aeubanks wrote:

does not having the build id change the header size?

No, the header will still contain the section for the build ID size. It's just that the value will be zero.

[aeubanks]

In D110277#3016320, @leonardchan wrote:

In D110277#3016205, @aeubanks wrote:

does not having the build id change the header size?

No, the header will still contain the section for the build ID size. It's just that the value will be zero.

I'm not understanding the description then. Why would the offset calculation be wrong if we're not changing the size of anything?

[phosek]

This test seems quite fragile, I'd prefer modifying it instead to compute the offset. It shouldn't be that difficult. You should be able to #include "profile/InstrProfData.inc" and then cast Buf to __llvm_profile_header and use it to compute the counter offset similarly to what's being done in https://github.com/llvm/llvm-project/blob/6e60bb6883178cf14e6fd47a6789495636e4322f/compiler-rt/lib/profile/InstrProfilingMerge.c#L95.

[leonardchan]

I'm not understanding the description then. Why would the offset calculation be wrong if we're not changing the size of anything?

Perhaps I should clarify in the description then. So after the header, we will also emit the binary IDs, then emit stuff like the data, names, and counters. If binary IDs are emitted, then there will be some extra data in between the header and the counters, which will shift the CounterPtr offset. Right now, with binary IDs, the calculation should be something like header_size + binary_ids_size + counter_offset. The calculation now is 11 * 8 + 2 * 8 only takes into account the header_size (11*8) and counter_offset (2*8) but not the binary_ids_size. If we don't emit binary ids though (via -Wl,--build-id=none), then binary_ids_size will be zero, making the calculation just header_size + counter_offset which matches what we currently have. The header will still contain the uint64_t referring to the binary ids size. It's just that its value will be zero. The alternative solution is to just explicitly calculate binary_ids_size which is what is suggested below.

In D110277#3016427, @phosek wrote:

This test seems quite fragile, I'd prefer modifying it instead to compute the offset. It shouldn't be that difficult. You should be able to #include "profile/InstrProfData.inc" and then cast Buf to __llvm_profile_header and use it to compute the counter offset similarly to what's being done in https://github.com/llvm/llvm-project/blob/6e60bb6883178cf14e6fd47a6789495636e4322f/compiler-rt/lib/profile/InstrProfilingMerge.c#L95.

If this seems more straightforward, then we can do that. I was just a bit lazy to update the test to account for this.

[phosek]

I'd prefer including profile/InstrProfData.inc and using types rather than just hardcoding the offsets. In the past, some fields have already moved around (for example between version 6 and 7 binary IDs size has moved). If this happens again, this test would break. Using types should be less error prone and more readable.

[aeubanks]

In D110277#3019229, @phosek wrote:

I'd prefer including profile/InstrProfData.inc and using types rather than just hardcoding the offsets. In the past, some fields have already moved around (for example between version 6 and 7 binary IDs size has moved). If this happens again, this test would break. Using types should be less error prone and more readable.

that sgtm

[phosek]

LGTM thanks!

compiler-rt/test/profile/Linux/corrupted-profile.c

 #include <stdlib.h>
 #include <string.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <unistd.h>
 
+enum ValueKind {
+#define VALUE_PROF_KIND(Enumerator, Value, Descr) Enumerator = Value,
+#include "profile/InstrProfData.inc"
+};
+
+typedef struct __llvm_profile_header {
+#define INSTR_PROF_RAW_HEADER(Type, Name, Initializer) Type Name;
+#include "profile/InstrProfData.inc"
+} __llvm_profile_header;
+
+typedef void *IntPtrT;
+typedef struct __llvm_profile_data {
+#define INSTR_PROF_DATA(Type, LLVMType, Name, Initializer) Type Name;
+#include "profile/InstrProfData.inc"
+} __llvm_profile_data;
 
 void __llvm_profile_set_file_object(FILE* File, int EnableMerge);
 
 void bail(const char* str) {
   fprintf(stderr, "%s %s\n", str, strerror(errno));
   exit(1);
 }
 
 int main(int argc, char** argv) {
   if (argc == 3) {
     int fd = open(argv[1], O_RDWR);
     if (fd == -1)
       bail("open");
 
     struct stat st;
     if (stat(argv[1], &st))
       bail("stat");
     uint64_t FileSize = st.st_size;
 
     char* Buf = (char *) mmap(NULL, FileSize, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
     if (Buf == MAP_FAILED)
       bail("mmap");
 
-    // We're trying to make the first CounterPtr invalid.
-    // 11 64-bit words as header.
-    // CounterPtr is the third 64-bit word field.
-    memset(&Buf[11 * 8 + 2 * 8], 0xAB, 8);
+    __llvm_profile_header *Header = (__llvm_profile_header *)Buf;
+    __llvm_profile_data *SrcDataStart =
+        (__llvm_profile_data *)(Buf + sizeof(__llvm_profile_header) +
+                                Header->BinaryIdsSize);
+    memset(&SrcDataStart->CounterPtr, 0xAB, sizeof(SrcDataStart->CounterPtr));
 
     if (munmap(Buf, FileSize))
       bail("munmap");
 
     if (close(fd))
       bail("close");
   } else {
     FILE* f = fopen(argv[1], "r+b");
     if (!f)
       bail("fopen");
     __llvm_profile_set_file_object(f, 1);
   }
 }