Page MenuHomePhabricator

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

Authored by grotdunst on Jan 20 2016, 1:40 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

grotdunst updated this revision to Diff 45411.Jan 20 2016, 1:40 PM
grotdunst retitled this revision from to [compiler-rt/profile] Added Hostname to .profdata file.
grotdunst updated this object.
grotdunst added a subscriber: llvm-commits.
davidxl accepted this revision.Jan 20 2016, 1:59 PM
davidxl edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jan 20 2016, 1:59 PM
vsk requested changes to this revision.Jan 28 2016, 10:31 AM
vsk added a reviewer: vsk.
vsk added a subscriber: 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?

This revision now requires changes to proceed.Jan 28 2016, 10:31 AM

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 updated this revision to Diff 46317.Jan 28 2016, 3:16 PM
grotdunst edited edge metadata.

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 added a comment.Jan 28 2016, 5:44 PM

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 edited edge metadata.Jan 28 2016, 6:10 PM
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.

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;

..

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 updated this revision to Diff 46436.Jan 29 2016, 2:52 PM
grotdunst edited edge metadata.

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

davidxl added inline comments.Jan 29 2016, 2:57 PM
lib/profile/InstrProfilingFile.c
159 ↗(On Diff #46436)

Is this check necessary?

grotdunst added inline comments.Jan 29 2016, 3:01 PM
lib/profile/InstrProfilingFile.c
159 ↗(On Diff #46436)

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

davidxl accepted this revision.Jan 29 2016, 3:06 PM
davidxl edited edge metadata.

lgtm with removing redundant check.

grotdunst updated this revision to Diff 46440.Jan 29 2016, 3:10 PM
grotdunst edited edge metadata.

Removed unnecessary condition test.

vsk accepted this revision.Jan 29 2016, 3:55 PM
vsk edited edge metadata.

Lgtm also, thanks Daniel!

This revision is now accepted and ready to land.Jan 29 2016, 3:55 PM
This revision was automatically updated to reflect the committed changes.