This is an archive of the discontinued LLVM Phabricator instance.

[libc++][CMake] Use debug MSVC runtimes when libc++ is built in debug mode
ClosedPublic

Authored by EricWF on Jan 13 2017, 10:59 PM.

Diff Detail

Event Timeline

EricWF updated this revision to Diff 84431.Jan 13 2017, 10:59 PM
EricWF retitled this revision from to [libc++][CMake] Use debug MSVC runtimes when libc++ is built in debug mode.
EricWF updated this object.
EricWF added reviewers: smeenai, rnk, majnemer, compnerd.
EricWF added a subscriber: cfe-commits.
EricWF updated this revision to Diff 84433.Jan 13 2017, 10:59 PM
smeenai edited edge metadata.Jan 13 2017, 11:06 PM

Awesome. LGTM except that force-removing the _DEBUG define and then linking against the debug libraries might cause some wonkiness.

CMakeLists.txt
394

We should be able to remove this now, right?

Awesome. LGTM except that force-removing the _DEBUG define and then linking against the debug libraries might cause some wonkiness.

Ack. I'll update the patch to still force remove _DEBUG but then later re-add it under the correct circumstances.

CMakeLists.txt
394

I would still rather strip it and re-add it. Just incase it gets added by LLVM or another parent project.

smeenai added inline comments.Jan 13 2017, 11:36 PM
CMakeLists.txt
394

Not completely sure I understand. Isn't this gonna be coming from cmake itself?

EricWF updated this revision to Diff 84437.Jan 13 2017, 11:39 PM
EricWF edited edge metadata.
  • Address inline comments.
  • Stop MSVC's default assertion handler from opening a new dialog window on failure.
EricWF added inline comments.Jan 13 2017, 11:43 PM
CMakeLists.txt
394

Yes, that was the original reason. Although it's possible an evil user could also add it.

I would rather handle it manually instead of depending on implicit default flags.

EricWF updated this revision to Diff 84438.Jan 13 2017, 11:45 PM

I think this patch is ready to go. @smeenai any last words?

smeenai added inline comments.Jan 13 2017, 11:55 PM
CMakeLists.txt
394

Ah, fair enough.

I think the /RTC1 doesn't need to be stripped out anymore, at least in debug mode (the extra checking might be useful).

test/support/set_windows_crt_report_mode.h
1 ↗(On Diff #84437)

Does this need the LLVM license header?

smeenai accepted this revision.Jan 13 2017, 11:57 PM
smeenai edited edge metadata.

LGTM apart from minor comments.

This revision is now accepted and ready to land.Jan 13 2017, 11:57 PM
EricWF updated this revision to Diff 84440.Jan 14 2017, 12:04 AM
EricWF edited edge metadata.
EricWF marked 2 inline comments as done.
  • Address inline comments.
EricWF closed this revision.Jan 14 2017, 12:05 AM