[compiler-rt/profile] Added Hostname to .profdata file

[grotdunst]

Summary

This revision facilitates the use of LLVM's instrumentation-based profiling within a cluster computing environment sharing a common file system. With the current compiler-rt, profiling data can be coherently gathered from multiple processors on the same node by having unique file names based on processor IDs. However, these files did not specify a hostname, meaning that in a shared file system processors from different nodes would write to the same file if they had the same PID. Through this revision, any inclusion of "%h" in a .profdata file name will be translated to the name of the host on which the program is run.

[davidxl]

LGTM

[vsk]

Hi Daniel, after taking a closer look I noticed a few problems. First, I don't think we should guard off new features if _MSC_VER is defined. Second, can't uname be used directly in place of %h in your lit test?

[davidxl]

Vedant brought up a good point -- which skipped me initially. We should not scatter #ifdef _MSC_VER in random files -- try to define portability macros in InstrProfilingPort.h.

[grotdunst]

This diff substitutes #ifdef COMPILER_RT_HOSTNAME for #ifndef _MSC_VER.

Second, can't uname be used directly in place of %h in your lit test?

This patch adds support for expanding %h and so it must be used in the test.

[vsk]

To clarify, my objection to using guards for this feature wasn't cosmetic: is there anything fundamental about MSVC that inhibits this feature?

I see that your lit test tests this patch, but (1) am concerned that the usage of uname might break Windows bots, and (2) wonder whether uname can be used by your scripts directly instead of relying on %h-expansion.

[hfinkel]

In D16371#338901, @vsk wrote:

To clarify, my objection to using guards for this feature wasn't cosmetic: is there anything fundamental about MSVC that inhibits this feature?

There's no uname() (or sys/utsname.h) on Windows. That's a POSIX function.

I see that your lit test tests this patch, but (1) am concerned that the usage of uname might break Windows bots, and

It would, except that the '// REQUIRES: shell' should cause the Windows bots to skip the test.

(2) wonder whether uname can be used by your scripts directly instead of relying on %h-expansion.

It is not clear how else to test that the host-name expansion actually works.

[davidxl]

Windows has native API gethostname to get host name. I suggest define portability macro

In InstrProfilingPort.h:

#define COMPILER_RT_MAX_HOSTLEN 128
#ifdef _MSC_VER
#define COMPILER_RT_GETHOSTNAME(Name, Len) gethostname(Name, Len)
#else
#define COMPILER_RT_GETHOSTNAME(Name, Len) GetHostName(Name, Len)
#define COMPILER_RT_HAS_UNAME 1
#endif

in InstrProfilingFile.c

#ifdef COMPILER_RT_HAS_UNAME
int GetHostName(char *Name, int Len) {

struct utsname N;
int R;
R = uname(&N);
memcpy(Name, N.nodename, Len);
return R;

}
#endif

The rest of the code will be greatly simplified:

char Name[COMPILER_RT_MAX_HOSTLEN];

if (COMPILER_RT_GETHOSTNAME(Name, COMPILER_RT_MAX_HOSTLEN))

return -1;

..

[hfinkel]

In D16371#339419, @davidxl wrote:

Windows has native API gethostname to get host name. I suggest define portability macro

In InstrProfilingPort.h:

#define COMPILER_RT_MAX_HOSTLEN 128
#ifdef _MSC_VER
#define COMPILER_RT_GETHOSTNAME(Name, Len) gethostname(Name, Len)
#else
#define COMPILER_RT_GETHOSTNAME(Name, Len) GetHostName(Name, Len)
#define COMPILER_RT_HAS_UNAME 1
#endif

in InstrProfilingFile.c

#ifdef COMPILER_RT_HAS_UNAME
int GetHostName(char *Name, int Len) {

struct utsname N;
int R;
R = uname(&N);
memcpy(Name, N.nodename, Len);

I think this needs to be strncpy, utsname.nodename is not guaranteed to be at least 128 characters. Also, we probably don't want to copy anything if the call to uname fails (so an early return would be better).

return R;

}
#endif

The rest of the code will be greatly simplified:

char Name[COMPILER_RT_MAX_HOSTLEN];

if (COMPILER_RT_GETHOSTNAME(Name, COMPILER_RT_MAX_HOSTLEN))
  return -1;

Otherwise, I agree.

[grotdunst]

Utilized Windows native API gethostname to get hostname. Modified macro definitions in both files for conciseness.

[davidxl]

lib/profile/InstrProfilingFile.c

151

Is this check necessary?

[grotdunst]

lib/profile/InstrProfilingFile.c

151

You're right: it's unnecessary because of getHostName. I'll remove it.

[davidxl]

lgtm with removing redundant check.

[grotdunst]

Removed unnecessary condition test.

[vsk]

Lgtm also, thanks Daniel!

lib/profile/InstrProfilingFile.c

 /*===- InstrProfilingFile.c - Write instrumentation to a file -------------===*\
 |*
 |*                     The LLVM Compiler Infrastructure
 |*
 |* This file is distributed under the University of Illinois Open Source
 |* License. See LICENSE.TXT for details.
 |*
 \*===----------------------------------------------------------------------===*/
 
 #include "InstrProfiling.h"
 #include "InstrProfilingInternal.h"
 #include "InstrProfilingUtil.h"
 #include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#ifdef COMPILER_RT_HOSTNAME
+#include <sys/utsname.h>
+#endif
 
 #define UNCONST(ptr) ((void *)(uintptr_t)(ptr))
 
 #ifdef _MSC_VER
 #define snprintf _snprintf
 #endif
 
 /* Return 1 if there is an error, otherwise return  0.  */

 }
 
 static void resetFilenameToDefault(void) { setFilename("default.profraw", 0); }
 
 int getpid(void);
 static int setFilenamePossiblyWithPid(const char *Filename) {
 #define MAX_PID_SIZE 16
   char PidChars[MAX_PID_SIZE] = {0};
-  int NumPids = 0, PidLength = 0;
+  int NumPids = 0, PidLength = 0, NumNodes = 0, NodeLength = 0;
   char *Allocated;
   int I, J;
+#ifdef COMPILER_RT_HOSTNAME
+  struct utsname Name;
+#endif
 
   /* Reset filename on NULL, except with env var which is checked by caller. */
   if (!Filename) {
     resetFilenameToDefault();
     return 0;
   }
 
   /* Check the filename for "%p", which indicates a pid-substitution. */
   for (I = 0; Filename[I]; ++I)
-    if (Filename[I] == '%' && Filename[++I] == 'p')
+    if (Filename[I] == '%') {
+      if (Filename[++I] == 'p') {
         if (!NumPids++) {
           PidLength = snprintf(PidChars, MAX_PID_SIZE, "%d", getpid());
           if (PidLength <= 0)
             return -1;
         }
-  if (!NumPids) {
+      } else if (Filename[I] == 'h') {
+        if (!NumNodes++)
+#ifdef COMPILER_RT_HOSTNAME
+          if (uname(&Name) == -1)
+            return -1;
+
+          NodeLength = strlen(Name.nodename);
+#endif
+          if (NodeLength <= 0)
+            return -1;
+      }
+    }
+
+  if (!(NumPids || NumNodes)) {
     setFilename(Filename, 0);
     return 0;
   }
 
   /* Allocate enough space for the substituted filename. */
-  Allocated = malloc(I + NumPids*(PidLength - 2) + 1);
+  Allocated = malloc(I + NumPids*(PidLength - 2) +
+                     NumNodes*(NodeLength - 2) + 1);
   if (!Allocated)
     return -1;
 
   /* Construct the new filename. */
   for (I = 0, J = 0; Filename[I]; ++I)
     if (Filename[I] == '%') {
       if (Filename[++I] == 'p') {
         memcpy(Allocated + J, PidChars, PidLength);
         J += PidLength;
       }
+#ifdef COMPILER_RT_HOSTNAME
+      else if (Filename[I] == 'h') {
+        memcpy(Allocated + J, Name.nodename, NodeLength);
+        J += NodeLength;
+      }
+#endif
       /* Drop any unknown substitutions. */
     } else
       Allocated[J++] = Filename[I];
   Allocated[J] = 0;
 
   /* Use the computed name. */
   setFilename(Allocated, 1);
   return 0;

lib/profile/InstrProfilingPort.h

 #elif __GNUC__
 #define COMPILER_RT_ALIGNAS(x) __attribute__((aligned(x)))
 #define COMPILER_RT_VISIBILITY __attribute__((visibility("hidden")))
 #define COMPILER_RT_WEAK __attribute__((weak))
 #endif
 
 #define COMPILER_RT_SECTION(Sect) __attribute__((section(Sect)))
 
+#ifndef _MSC_VER
+#define COMPILER_RT_HOSTNAME 1
+#endif
+
 #if COMPILER_RT_HAS_ATOMICS == 1
 #ifdef _MSC_VER
 #include <windows.h>
 #if defined(_WIN64)
 #define COMPILER_RT_BOOL_CMPXCHG(Ptr, OldV, NewV)                              \
   (InterlockedCompareExchange64((LONGLONG volatile *)Ptr, (LONGLONG)NewV,      \
                                 (LONGLONG)OldV) == (LONGLONG)OldV)
 #else /* !defined(_WIN64) */

test/profile/instrprof-hostname.c

+// RUN: %clang_profgen -o %t -O3 %s
+// RUN: env LLVM_PROFILE_FILE=%h.%t-%h.profraw_%h %run %t
+// RUN: llvm-profdata merge -o %t.profdata `uname -n`.%t-`uname -n`.profraw_`uname -n`
+// RUN: %clang_profuse=%t.profdata -o - -S -emit-llvm %s | FileCheck %s
+// REQUIRES: shell
+
+int main(int argc, const char *argv[]) {
+  // CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}, !prof ![[PD1:[0-9]+]]
+  if (argc > 2)
+    return 1;
+  return 0;
+}
+// CHECK: ![[PD1]] = !{!"branch_weights", i32 1, i32 2}