Page MenuHomePhabricator

[PGO] Enable building compiler-rt profile support library on Windows
ClosedPublic

Authored by slingn on Dec 30 2015, 1:54 PM.

Details

Summary

This change configures Windows builds to build the complier-rt profile support library (clang_rt.profile-i386.lib). Windows API incompatibilities in the compiler-rt profile lib are also fixed.

Diff Detail

Event Timeline

slingn updated this revision to Diff 43807.Dec 30 2015, 1:54 PM
slingn retitled this revision from to [PGO] Enable instrprof on Windows.
slingn updated this object.
slingn added reviewers: davidxl, dnovillo.
slingn added a subscriber: llvm-commits.
slingn retitled this revision from [PGO] Enable instrprof on Windows to [PGO] Enable building compiler-rt profile support library on Windows.Dec 30 2015, 1:57 PM
slingn updated this object.
slingn updated this revision to Diff 43809.Dec 30 2015, 1:58 PM

Updated commit message.

davidxl added inline comments.Dec 30 2015, 2:51 PM
lib/profile/mmap-windows.c
2 ↗(On Diff #43809)

This file needs to be re-licensed and redistributed under LLVM* license. need to talk to the contributor of the file and get the permission.

124 ↗(On Diff #43809)

better use C style comments -- also in many other places.

slingn added inline comments.Dec 30 2015, 3:08 PM
lib/profile/mmap-windows.c
2 ↗(On Diff #43809)

I thought public domain means that there are no restrictions whatsoever - including re-licensing, selling, etc. No need for attribution even.

I modeled this after the MD5 attribution:
llvm/include/llvm/Support/MD5.h

slingn updated this revision to Diff 43813.Dec 30 2015, 3:15 PM
slingn marked an inline comment as done.

Fix C++ -> C style comments in mmap-windows.c compatibility file.

davidxl edited edge metadata.Dec 30 2015, 3:30 PM

Is there a missing change in CMakeLists.txt for the new file added?

mmap-windows.c gets #included directly in GCDAProfiling.c since the compatibility mmap()/msync()/flock() functions are not used anywhere else (they are static).

I agree it is a bit odd - and CMake won't properly detect changes to mmap-windows.c the way things are now. Perhaps it would be better to mark them with COMPILER_RT_VISIBILITY (and not static).

slingn updated this revision to Diff 43931.Jan 4 2016, 3:14 PM
slingn edited edge metadata.

Updated for davidxl's comments

-refactor Windows mmap compatibility code to have a header file, not include implementation with static functions directly
-switch to C-style comments in Windows mmap compatibility code

slingn updated this revision to Diff 43933.Jan 4 2016, 3:17 PM

Fix header comment.

davidxl added inline comments.Jan 4 2016, 4:08 PM
lib/profile/CMakeLists.txt
40

This is not needed if the source file is properly guarded -- see other platform specific files.

lib/profile/WindowsMMap.c
21

Move this line above the first include.

slingn updated this revision to Diff 43943.Jan 4 2016, 4:37 PM
slingn marked 2 inline comments as done.

-Fixed guard of WindowsMMap.c

davidxl accepted this revision.Jan 4 2016, 4:38 PM
davidxl edited edge metadata.

lgtm

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