allocator cleanup: Reorganize allocator.gyp for cleanup

base/allocator/allocator.gyp

Index: base/allocator/allocator.gyp
diff --git a/base/allocator/allocator.gyp b/base/allocator/allocator.gyp
index ae93e9e03ca902fb8a10e66e1c2230f47a7d8ae7..174394bc56046ad9fb769f2d72f4cd304caff0d3 100644
--- a/base/allocator/allocator.gyp
+++ b/base/allocator/allocator.gyp
@@ -24,69 +24,73 @@
     {
       'target_name': 'allocator',
       'type': 'static_library',

[Nico, 2015/12/02 21:32:21]

Maybe this should be type none by default and only type static_library in cases
where it's real? Else you end up with a source-less static_library target which
confuses at least some generators (but I suppose that's not new in this CL)

[Primiano Tucci (use gerrit), 2015/12/03 10:00:55]

OK, if you agree let's do this. I'm putting a TODO right now and will do once I
make the outer pass on the other gyp files and remove most of the conditionals
inside here.
Rationale: right now the only way I see for doing that is to either:
 - have a condition section for the type which is the union of the conditions
below.
 - Have the type: static library inside each conditionals.
Both of them look a bit fragile and easy to get wrong during the refactorings.
Ideally I'd like to end up in a stage here where:
if use_allocator == none -> target type = none

However this is not the situation today, as use_allocator right now (with or
without these refactorings) is a big lie which just sometimes happens to be
true.

-      'direct_dependent_settings': {
-        'configurations': {
-          'Common_Base': {
-            'msvs_settings': {
-              'VCLinkerTool': {
-                'IgnoreDefaultLibraryNames': ['libcmtd.lib', 'libcmt.lib'],
-                'AdditionalDependencies': [
-                  '<(SHARED_INTERMEDIATE_DIR)/allocator/libcmt.lib'
-                ],
+      'conditions': [
+        ####################################################################
+        # WINDOWS SHIM LAYER
+        ####################################################################

[Nico, 2015/12/02 21:32:21]

###########################################################
# MAYBE THIS COULD BE IN LOWER CASE; NOT SURE THE #### LINES
# ADD MUCH EITHER
###########################################################

[Primiano Tucci (use gerrit), 2015/12/03 10:00:55]

Ok, ok, agree. this was mostly driven my yesterday frustration in scrolling this
file up and down all the day.

+        ['OS=="win" and win_use_allocator_shim==1', {
+          'msvs_settings': {
+            # TODO(sgk):  merge this with build/common.gypi settings
+            'VCLibrarianTool': {
+              'AdditionalOptions': ['/ignore:4006,4221'],
+            },
+            'VCLinkerTool': {
+              'AdditionalOptions': ['/ignore:4006'],
+            },
+          },
+          'dependencies': [
+            'libcmt',
+          ],
+          'include_dirs': [
+            '../..',
+          ],
+          'sources': [
+            'allocator_shim_win.cc',
+          ],
+          'configurations': {
+            'Debug_Base': {
+              'msvs_settings': {
+                'VCCLCompilerTool': {
+                  'RuntimeLibrary': '0',
+                },
               },
             },
           },
-        },
-        'conditions': [
-          ['OS=="win"', {
+          'direct_dependent_settings': {
+            'configurations': {
+              'Common_Base': {
+                'msvs_settings': {
+                  'VCLinkerTool': {
+                    'IgnoreDefaultLibraryNames': ['libcmtd.lib', 'libcmt.lib'],
+                    'AdditionalDependencies': [
+                      '<(SHARED_INTERMEDIATE_DIR)/allocator/libcmt.lib'
+                    ],
+                  },
+                },
+              },
+            },
             'defines': [
               'PERFTOOLS_DLL_DECL=',
             ],
-          }],
-        ],
-      },
-      'dependencies': [
-        '../third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations',
-      ],
-      'msvs_settings': {
-        # TODO(sgk):  merge this with build/common.gypi settings
-        'VCLibrarianTool': {
-          'AdditionalOptions': ['/ignore:4006,4221'],
-        },
-        'VCLinkerTool': {
-          'AdditionalOptions': ['/ignore:4006'],
-        },
-      },
-      'configurations': {
-        'Debug_Base': {
-          'msvs_settings': {
-            'VCCLCompilerTool': {
-              'RuntimeLibrary': '0',
-            },
           },
-          'variables': {
-            # Provide a way to force disable debugallocation in Debug builds,
-            # e.g. for profiling (it's more rare to profile Debug builds,
-            # but people sometimes need to do that).
-            'disable_debugallocation%': 0,
-          },
-          'conditions': [
-            ['disable_debugallocation==0', {
-              'defines': [
-                # Use debugallocation for Debug builds to catch problems early
-                # and cleanly, http://crbug.com/30715 .
-                'TCMALLOC_FOR_DEBUGALLOCATION',
-              ],
-            }],
-          ],
-        },
-      },
-      'conditions': [
-        ['use_allocator=="tcmalloc"', {
+        }],  # OS=="win"
+        ####################################################################
+        # TCMALLOC
+        ####################################################################
+        # TODO(primiano): remove the per-os conditionals one common.gypi is

[Nico, 2015/12/02 21:32:21]

s/one/once

[Primiano Tucci (use gerrit), 2015/12/03 10:00:55]

Done.

+        # fixed (i.e. mac stops saying use_allocator=tcmalloc). One should be
+        # able to say use_allocator=tcmalloc on Mac and as a result this should
+        # at least try to build it, not silently ignore that.
+        ['use_allocator=="tcmalloc" and os_posix==1 and OS!="mac" and OS!="ios"', {
           # Disable the heap checker in tcmalloc.
           'defines': [
             'NO_HEAP_CHECK',
           ],
+          'dependencies': [
+            '../third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations',
+          ],
+          # The order of this include_dirs matters, as tc-malloc has its own
+          # base/ mini-fork. Do not factor these out in the allocator target.

[Nico, 2015/12/02 21:32:21]

I'm not sure what the second sentence means. We are in the allocator target. "do
not factor these out of"?

[Primiano Tucci (use gerrit), 2015/12/03 10:00:55]

Reworded. What I mean here is that it would be temping to say:
include_dir: ['.', '../..'] in the allocator target, outside of the conditions
(rationale: this is the same for win shim),
and here (inside this tcmalloc condition) just add the two <(tcmalloc_dir)
includes.
But that doesn't work, so each condition section has to repeat the include paths
(unless there is some gyp trick to enforce the order)

           'include_dirs': [
             '.',
             '<(tcmalloc_dir)/src/base',
@@ -287,59 +291,72 @@
             # Included by debugallocation_shim.cc.
             '<(tcmalloc_dir)/src/debugallocation.cc',
             '<(tcmalloc_dir)/src/tcmalloc.cc',
-          ]
-        },{
-          'include_dirs': [
-            '.',
-            '../..',
-          ],
-        }],
-        ['OS=="win" and component!="shared_library"', {
-          'dependencies': [
-            'libcmt',

[Nico, 2015/12/02 21:32:21]

where did this go?

[Primiano Tucci (use gerrit), 2015/12/03 10:00:55]

On line 42.
You could argue: "wait but !shared_library is not the same of
win_use_allocator_shim" (in fact use_shim is 1 only if static + MSVS2013), while
!shared_library is true also for MSVS 2015 static builds.
However this entire target is depended on by the various win targets using an if
(win and win_use_allocator_shim), so this more general condition should never be
hit and be bool-equivalent to checking for win_shim.

As a matter of facts, I will add the MSVS=2015 to the list of the ninja diff
cross-checks.

-          ],
-          'sources': [
-            'allocator_shim_win.cc',
-          ],
-        }],
-        ['profiling!=1', {
-          'sources!': [
-            # cpuprofiler
-            '<(tcmalloc_dir)/src/base/thread_lister.c',
-            '<(tcmalloc_dir)/src/base/thread_lister.h',
-            '<(tcmalloc_dir)/src/profile-handler.cc',
-            '<(tcmalloc_dir)/src/profile-handler.h',
-            '<(tcmalloc_dir)/src/profiledata.cc',
-            '<(tcmalloc_dir)/src/profiledata.h',
-            '<(tcmalloc_dir)/src/profiler.cc',
-          ],
-        }],
-        ['OS=="linux" or OS=="freebsd" or OS=="solaris" or OS=="android"', {
-          'sources!': [
-            '<(tcmalloc_dir)/src/system-alloc.h',
           ],
-          # We enable all warnings by default, but upstream disables a few.
-          # Keep "-Wno-*" flags in sync with upstream by comparing against:
-          # http://code.google.com/p/google-perftools/source/browse/trunk/Makefile.am
-          'cflags': [
-            '-Wno-sign-compare',
-            '-Wno-unused-result',
-          ],
-          'cflags!': [
-            '-fvisibility=hidden',
+          'conditions': [
+            ['OS=="linux" or OS=="freebsd" or OS=="solaris" or OS=="android"', {
+              'sources!': [
+                '<(tcmalloc_dir)/src/system-alloc.h',
+              ],
+              # We enable all warnings by default, but upstream disables a few.
+              # Keep "-Wno-*" flags in sync with upstream by comparing against:
+              # http://code.google.com/p/google-perftools/source/browse/trunk/Makefile.am
+              'cflags': [
+                '-Wno-sign-compare',
+                '-Wno-unused-result',
+              ],
+              'cflags!': [
+                '-fvisibility=hidden',
+              ],
+              'link_settings': {
+                'ldflags': [
+                  # Don't let linker rip this symbol out, otherwise the heap&cpu
+                  # profilers will not initialize properly on startup.
+                  '-Wl,-uIsHeapProfilerRunning,-uProfilerStart',
+                  # Do the same for heap leak checker.
+                  '-Wl,-u_Z21InitialMallocHook_NewPKvj,-u_Z22InitialMallocHook_MMapPKvS0_jiiix,-u_Z22InitialMallocHook_SbrkPKvi',
+                  '-Wl,-u_Z21InitialMallocHook_NewPKvm,-u_Z22InitialMallocHook_MMapPKvS0_miiil,-u_Z22InitialMallocHook_SbrkPKvl',
+                  '-Wl,-u_ZN15HeapLeakChecker12IgnoreObjectEPKv,-u_ZN15HeapLeakChecker14UnIgnoreObjectEPKv',
+                ],
+              },
+            }],
+            ['profiling!=1', {
+              'sources!': [
+                # cpuprofiler
+                '<(tcmalloc_dir)/src/base/thread_lister.c',
+                '<(tcmalloc_dir)/src/base/thread_lister.h',
+                '<(tcmalloc_dir)/src/profile-handler.cc',
+                '<(tcmalloc_dir)/src/profile-handler.h',
+                '<(tcmalloc_dir)/src/profiledata.cc',
+                '<(tcmalloc_dir)/src/profiledata.h',
+                '<(tcmalloc_dir)/src/profiler.cc',
+              ],
+            }],
           ],
-          'link_settings': {
-            'ldflags': [
-              # Don't let linker rip this symbol out, otherwise the heap&cpu
-              # profilers will not initialize properly on startup.
-              '-Wl,-uIsHeapProfilerRunning,-uProfilerStart',
-              # Do the same for heap leak checker.
-              '-Wl,-u_Z21InitialMallocHook_NewPKvj,-u_Z22InitialMallocHook_MMapPKvS0_jiiix,-u_Z22InitialMallocHook_SbrkPKvi',
-              '-Wl,-u_Z21InitialMallocHook_NewPKvm,-u_Z22InitialMallocHook_MMapPKvS0_miiil,-u_Z22InitialMallocHook_SbrkPKvl',
-              '-Wl,-u_ZN15HeapLeakChecker12IgnoreObjectEPKv,-u_ZN15HeapLeakChecker14UnIgnoreObjectEPKv',
-          ]},
-        }],
-        [ 'use_vtable_verify==1', {
+          'configurations': {
+            'Debug_Base': {
+              'variables': {
+                # Provide a way to force disable debugallocation in Debug builds
+                # e.g. for profiling (it's more rare to profile Debug builds,
+                # but people sometimes need to do that).
+                'disable_debugallocation%': 0,

[Nico, 2015/12/02 21:32:21]

moving this into a configuration subtly changes behavior on when this is merge.
If some gyp file set this to 0 in a gypi, I think this would now no longer
overwrite this default var. Using GYP_DEFINES to set it still works. Probably
doesn't matter.

[Primiano Tucci (use gerrit), 2015/12/03 10:00:55]

I can move it to the variables on top, but I think that then I have to do the
variables { variables { variables trick to make it expand here.
Honestly, not sure who still uses this, and am pretty sure that this is only
intended to be passed by GYP_DEFINES (don't see this to be mentioned anywhere
else)

+              },
+              'conditions': [
+                ['disable_debugallocation==0', {
+                  'defines': [
+                    # Use debugallocation for Debug builds to catch problems
+                    # early and cleanly, http://crbug.com/30715 .
+                    'TCMALLOC_FOR_DEBUGALLOCATION',
+                  ],
+                }],
+              ],
+            },
+          },
+        }],  # use_allocator=="tcmalloc
+        # For CrOS builds with vtable verification. According to the author of
+        # crrev.com/10854031 this is used in conjuction with some other CrOS
+        # build flag, to enable verification of any allocator that uses virtual
+        # function calls.
+        ['use_vtable_verify==1', {
           'cflags': [
             '-fvtable-verify=preinit',
           ],
@@ -351,8 +368,8 @@
             }],
           ],
         }],
-      ],
-    },
+      ],  # condditions of 'allocator' target.

[Nico, 2015/12/02 21:32:21]

s/condditions/conditions

[Primiano Tucci (use gerrit), 2015/12/03 10:00:55]

Done.

+    },  # 'allocator' target.
     {
       # This library is linked in to src/base.gypi:base and allocator_unittests
       # It can't depend on either and nothing else should depend on it - all