Fix missing CRT libraries with ASAN unittests

Fix missing CRT libraries with ASAN unittests

Some targets didn't compiled when instrumenting with ASAN.
This is happening with when removing the default libs.
The ASAN runtime needs some CRT functions.

R=rnk@chromium.org

BUG=636398

[etienneb, 2016-12-14 15:32:14]

Description was changed from

==========
Fix missing CRT libraries with ASAN unittests

Some targets didn't compiled when instrumenting with ASAN.
This is happening with when removing the default libs.
The ASAN runtime needs some CRT functions.

R=rnk@chromium.org

BUG=
==========

to

==========
Fix missing CRT libraries with ASAN unittests

Some targets didn't compiled when instrumenting with ASAN.
This is happening with when removing the default libs.
The ASAN runtime needs some CRT functions.

R=rnk@chromium.org

BUG=
==========

[etienneb, 2016-12-14 15:32:14]

etienneb@chromium.org changed reviewers:
+ dpranke@chromium.org, thakis@chromium.org

[etienneb, 2016-12-14 15:32:26]

PTAL.

[Nico, 2016-12-14 15:51:31]

this is for https://bugs.chromium.org/p/chromium/issues/detail?id=636398 yes?

[etienneb, 2016-12-14 15:53:50]

On 2016/12/14 15:51:31, Nico wrote:
> this is for https://bugs.chromium.org/p/chromium/issues/detail?id=636398 yes?

I didn't saw the bug, but yes, it's fixing this issue.

[etienneb, 2016-12-14 15:54:38]

On 2016/12/14 15:53:50, etienneb wrote:
> On 2016/12/14 15:51:31, Nico wrote:
> > this is for https://bugs.chromium.org/p/chromium/issues/detail?id=636398
yes?
> 
> I didn't saw the bug, but yes, it's fixing this issue.

Apparently, I reported that bug a while ago! Wow.

[Nico, 2016-12-14 15:55:03]

Description was changed from

==========
Fix missing CRT libraries with ASAN unittests

Some targets didn't compiled when instrumenting with ASAN.
This is happening with when removing the default libs.
The ASAN runtime needs some CRT functions.

R=rnk@chromium.org

BUG=
==========

to

==========
Fix missing CRT libraries with ASAN unittests

Some targets didn't compiled when instrumenting with ASAN.
This is happening with when removing the default libs.
The ASAN runtime needs some CRT functions.

R=rnk@chromium.org

BUG=636398
==========

[Nico, 2016-12-14 15:58:46]

I'll leave it to Reid and you to decide if users of asan that use /ENTRY: need
to manually pass these, or if we could do this automatically in asan somehow. In
comment 7 on the bug I point out that we miss mainCRTStartup with this approach,
which may or may not be an issue. (But if we do go this path, maybe Reid's
pragma-comment-lib suggestion is better as it doesn't require clients to do
this?)

Anyhoo: If this is the path we want to take, then I think you should instead
make a custom_entry config that does both pass /ENTRY and in asan flags the crt
flags, so that people who want to set /ENTRY can use that config and then get
the right asan behavior automatically.

[Reid Kleckner, 2016-12-14 20:01:51]

On 2016/12/14 15:58:46, Nico wrote:
> I'll leave it to Reid and you to decide if users of asan that use /ENTRY: need
> to manually pass these, or if we could do this automatically in asan somehow.
In
> comment 7 on the bug I point out that we miss mainCRTStartup with this
approach,
> which may or may not be an issue. (But if we do go this path, maybe Reid's
> pragma-comment-lib suggestion is better as it doesn't require clients to do
> this?)

I don't really like the #pragma comment lib approach because it can be defeated
with /NODEFAULTLIB:. Basically, ASan needs the CRT, so users shouldn't be
disabling the CRT when ASan is enabled. It's kind of like crashpad and ASan
fighting over the unhandled exception filter. At some point, ASan can't do
everything transparently, and we have to stop escalating and change the user
code.

So, I think the functionality here is correct. Does anyone (Dirk) have
suggestions for how to simplify or regularize the gn configs here, though?

[etienneb, 2016-12-14 20:03:12]

etienneb@chromium.org changed reviewers:
+ chrisha@chromium.org

[Dirk Pranke, 2016-12-15 03:10:17]

I haven't had a chance to look at this yet, sorry! I will try to get to it
tomorrow.

[Dirk Pranke, 2016-12-18 04:20:49]

lgtm; this patch looks pretty straightforward as-is, GN-wise.

https://codereview.chromium.org/2570153002/diff/40001/build/config/sanitizers...
File build/config/sanitizers/BUILD.gn (right):

https://codereview.chromium.org/2570153002/diff/40001/build/config/sanitizers...
build/config/sanitizers/BUILD.gn:302: # librairies using the linker options
"/NOENTRY", "/ENTRY" or "/NODEFAULTLIB".
s/librairies/libraries/

[etienneb, 2017-01-10 16:21:58]

etienneb@chromium.org changed reviewers:
+ robertshield@chromium.org

[etienneb, 2017-01-10 16:30:59]

etienneb@chromium.org changed reviewers:
+ sergeyu@chromium.org

[etienneb, 2017-01-10 16:32:15]

Could owners lgtm these files
  sergey : remoting/host/win/BUILD.gn
  robert : chrome/installer/mini_installer/BUILD.gn

[joedow, 2017-01-10 16:40:21]

joedow@chromium.org changed reviewers:
+ joedow@chromium.org

[joedow, 2017-01-10 16:40:22]

https://codereview.chromium.org/2570153002/diff/40001/remoting/host/win/BUILD.gn
File remoting/host/win/BUILD.gn (right):

https://codereview.chromium.org/2570153002/diff/40001/remoting/host/win/BUILD...
remoting/host/win/BUILD.gn:275: "//build/config/sanitizers:deps",
Just curious, why was deps updated for this target but not for the others that
had the sanitizer_default_libs added to the config list?

[robertshield, 2017-01-10 18:22:27]

mini_installer lgtm

[Sergey Ulanov, 2017-01-10 20:32:09]

lgtm

[etienneb, 2017-01-11 16:05:33]

done

https://codereview.chromium.org/2570153002/diff/40001/build/config/sanitizers...
File build/config/sanitizers/BUILD.gn (right):

https://codereview.chromium.org/2570153002/diff/40001/build/config/sanitizers...
build/config/sanitizers/BUILD.gn:302: # librairies using the linker options
"/NOENTRY", "/ENTRY" or "/NODEFAULTLIB".
On 2016/12/18 04:20:49, Dirk Pranke wrote:
> s/librairies/libraries/

Done.

https://codereview.chromium.org/2570153002/diff/40001/remoting/host/win/BUILD.gn
File remoting/host/win/BUILD.gn (right):

https://codereview.chromium.org/2570153002/diff/40001/remoting/host/win/BUILD...
remoting/host/win/BUILD.gn:275: "//build/config/sanitizers:deps",
On 2017/01/10 16:40:22, joedow wrote:
> Just curious, why was deps updated for this target but not for the others that
> had the sanitizer_default_libs added to the config list?

This is adding sanitizers code into the executable. Here is both linker response
files (without and with the line):


out\ninja\remoting_console.exe.rsp

[without :deps]

obj/remoting/host/win/remoting_console/version.res
obj/remoting/host/win/remoting_console/entry_point.obj
obj/remoting/host/win/dpi_aware_exe_manifest/dpi_aware_exe_manifest.manifest.res
remoting_core.dll.lib
advapi32.lib comdlg32.lib dbghelp.lib delayimp.lib dnsapi.lib gdi32.lib
kernel32.lib msimg32.lib odbc32.lib odbccp32.lib ole32.lib oleaut32.lib
psapi.lib shell32.lib shlwapi.lib user32.lib usp10.lib uuid.lib version.lib
wininet.lib winmm.lib winspool.lib ws2_32.lib clang_rt.asan-i386.lib libcmt.lib
libucrt.lib libvcruntime.lib
/ENTRY:HostEntryPoint /OPT:REF /OPT:ICF /INCREMENTAL:NO /FIXED:NO /WX /OPT:ICF
/DEBUG /MACHINE:X86 /SAFESEH /largeaddressaware /fastfail /FIXED:NO /ignore:4199
/ignore:4221 /NXCOMPAT /DYNAMICBASE /INCREMENTAL:NO /SUBSYSTEM:CONSOLE,5.01 
/DELAYLOAD:cfgmgr32.dll /DELAYLOAD:powrprof.dll /DELAYLOAD:setupapi.dll
/DELAYLOAD:mf.dll /DELAYLOAD:mfplat.dll /DELAYLOAD:mfreadwrite.dll
/DELAYLOAD:BluetoothApis.dll /DELAYLOAD:Bthprops.cpl /DELAYLOAD:setupapi.dll 
/LIBPATH:d:/src/llvm/ninja/lib/clang/4.0.0/lib/windows 
/LIBPATH:D:/src/depot_tools/win_toolchain/vs_files/d3cb0e37bdd120ad0ac4650b674b09e81be45616/win_sdk/Lib/winv6.3/um/x86

/LIBPATH:D:/src/depot_tools/win_toolchain/vs_files/d3cb0e37bdd120ad0ac4650b674b09e81be45616/VC/lib
/LIBPATH:D:/src/depot_tools/win_toolchain/vs_files/d3cb0e37bdd120ad0ac4650b674b09e81be45616/VC/atlmfc/lib


[with :deps]

obj/remoting/host/win/remoting_console/version.res
obj/remoting/host/win/remoting_console/entry_point.obj
obj/remoting/host/win/dpi_aware_exe_manifest/dpi_aware_exe_manifest.manifest.res
remoting_core.dll.lib 
obj/build/config/sanitizers/options_sources.lib <<<-----------------
advapi32.lib comdlg32.lib dbghelp.lib delayimp.lib dnsapi.lib gdi32.lib
kernel32.lib msimg32.lib odbc32.lib odbccp32.lib ole32.lib oleaut32.lib
psapi.lib shell32.lib shlwapi.lib user32.lib usp 10.lib uuid.lib version.lib
wininet.lib winmm.lib winspool.lib ws2_32.lib clang_rt.asan-i386.lib libcmt.lib
libucrt.lib  libvcruntime.lib
/ENTRY:HostEntryPoint /OPT:REF /OPT:ICF /INCREMENTAL:NO /FIXED:NO /WX /OPT:ICF
/DEBUG /MACHINE:X86 /SAFESEH /largeaddressaware /fastfail /FIXED:NO /ignore:4199
/ignore:4221 /NXCOMPAT /DYNAMICBASE /INCREMENTAL:NO /SUBSYSTEM:CONSOLE,5.01
/DELAYLOAD:cfgmgr32.dll /DELAYLOAD:powrprof.dll /DELAYLOAD:setupapi.dll
/DELAYLOAD:mf.dll /DELAYLOAD:mfplat.dll /DELAYLOAD:mfreadwrite.dll
/DELAYLOAD:BluetoothApis.dll /DELAYLOAD:Bthprops.cpl /DELAYLOAD:setupapi.dll
/LIBPATH:d:/src/llvm/ninja/lib/clang/4.0.0/lib/windows 
/LIBPATH:D:/src/depot_tools/win_toolchain/vs_files/d3cb0e37bdd120ad0ac4650b674b09e81be45616/win_sdk/Lib/winv6.3/um/x86

/LIBPATH:D:/src/depot_tools/win_toolchain/vs_files/d3cb0e37bdd120ad0ac4650b674b09e81be45616/VC/lib

/LIBPATH:D:/src/depot_tools/win_toolchain/vs_files/d3cb0e37bdd120ad0ac4650b674b09e81be45616/VC/atlmfc/lib



It's adding this file: obj/build/config/sanitizers/options_sources.lib 
built from: //build/sanitizers/sanitizer_options.cc

Which brings default ASAN options used by chromium project.

  const char kAsanDefaultOptions[] =
      "legacy_pthread_cond=1 malloc_context_size=5 "
      "symbolize=1 check_printf=1 use_sigaltstack=1  detect_leaks=0 "
      "strip_path_prefix=/../../ fast_unwind_on_fatal=1";



Without the ":deps" entry, these executables are compiling anyway. I suspect
this will cause issue with the components build because ":deps" is bringing:

  config("default_sanitizer_ldflags")

Which is adding the required ASAN libs:

      if (is_component_build) {
        assert(false, "win/asan does not work in 64-bit yet")
        libs = [
          "clang_rt.asan_dynamic-x86_64.lib",
          "clang_rt.asan_dynamic_runtime_thunk-x86_64.lib",
        ]
      }

I'll bring back this line in a separate CL as multiple targets may have the same
issue.

[etienneb, 2017-01-11 16:05:51]

The CQ bit was checked by etienneb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 16:06:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[etienneb, 2017-01-19 15:35:30]

This patch got replaced by this fix:
  https://codereview.chromium.org/2621223003/

closing it.