This is an archive of the discontinued LLVM Phabricator instance.

make -fprofile-instr-generate and -fprofile-instr-use work using clang-cl
ClosedPublic

Authored by inglorion on Nov 23 2016, 10:16 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

inglorion updated this revision to Diff 79186.Nov 23 2016, 10:16 PM
inglorion retitled this revision from to made -fprofile-instr-generate and -fprofile-instr-use work on Windows and using clang-cl.
inglorion updated this object.
inglorion added reviewers: amccarth, hans, rnk, ruiu, zturner.
amccarth added inline comments.Nov 28 2016, 8:04 AM
lib/Driver/ToolChain.cpp
538 ↗(On Diff #79186)

Which code uses gethostname?

The only places I find it are in Unix-specific #ifdefs.

I'm mildly concerned that we're requiring a working network connection to do profiling. The winsock implementation of gethostname will fail if there is no active network connection (or a variety of other reasons).

If you just need the name of the computer, it might be better to use GetComputerNameEx on Windows.

inglorion added inline comments.Nov 28 2016, 8:44 AM
lib/Driver/ToolChain.cpp
538 ↗(On Diff #79186)

Yeah, I don't like it, either. My guess is some code in compiler-rt uses gethostname to allow files to be named after the host. We might want to go in and change that, as you say, but it would be unrelated to this patch.

amccarth added inline comments.Nov 28 2016, 8:47 AM
lib/Driver/ToolChain.cpp
538 ↗(On Diff #79186)

But where is that code? I'm doing a tree-wide search, and I don't see a case where gethostbyname would be called on Windows.

rnk added inline comments.Nov 28 2016, 8:47 AM
lib/Driver/ToolChain.cpp
538 ↗(On Diff #79186)

How about using #pragma comment(lib, "ws2_32") in the profiling runtime sources? It's a Windows-ism, but it has the nice property that it keeps the system library knowledge close to the site of the usage.

inglorion added inline comments.Nov 28 2016, 10:25 AM
lib/Driver/ToolChain.cpp
538 ↗(On Diff #79186)

I think it's probably line 44 in projects/compiler-rt/lib/profile/InstrProfilingPort.h

inglorion added inline comments.Nov 28 2016, 4:28 PM
lib/Driver/ToolChain.cpp
538 ↗(On Diff #79186)

With D27178, we wouldn't need to link in ws2_32 anymore.

inglorion updated this revision to Diff 79484.Nov 28 2016, 5:06 PM

Removed the code that links ws2_32, which we won't need anymore after D27178 lands.

rnk accepted this revision.Nov 28 2016, 5:11 PM
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.Nov 28 2016, 5:11 PM
hans accepted this revision.Nov 28 2016, 7:01 PM
hans edited edge metadata.

lgtm modulo comment about -- in the test file.

Also I'd suggest phrasing the commit message in the imperative (and maybe drop the "make .. work on windows" now that the patch is really just about exposing the flags in clang-cl).

test/Driver/cl-options.c
62 ↗(On Diff #79484)

Please put -- before %s here and below (see note at the top of the file).

inglorion updated this revision to Diff 79691.Nov 29 2016, 6:49 PM
inglorion retitled this revision from made -fprofile-instr-generate and -fprofile-instr-use work on Windows and using clang-cl to make -fprofile-instr-generate and -fprofile-instr-use work using clang-cl.
inglorion updated this object.
inglorion edited edge metadata.

Changes requested by @hans (thanks!)

This revision was automatically updated to reflect the committed changes.