Explictly pass -marm when arm mode is required

Explictly pass -marm when arm mode is required

Simply disabling the -mthumb flag is not enough
because the chromeos toolchain defaults to thumb.

BUG=564059
Committed: https://crrev.com/c0e12589bf7ae45f0f463d4726224442af90e142
Cr-Commit-Position: refs/heads/master@{#435919}

[Sam Clegg, 2016-11-22 20:00:32]

sbc@chromium.org changed reviewers:
+ brettw@chromium.org, dpranke@chromium.org, llozano@chromium.org

[llozano, 2016-11-22 22:46:24]

On 2016/11/22 20:00:32, Sam Clegg wrote:

I have tried this and it works for me. 
I had to apply the patch in https://codereview.chromium.org/2457473003/ for the
problem to reproduce.
And after applying this change, the problem goes away. -marm is added to malloc
compilations.

[Sam Clegg, 2016-11-23 18:34:33]

PTAL.

Even though trybot is red, the compile phase is green and this bots test phases
having been green in forever.

[Sam Clegg, 2016-11-23 18:34:44]

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

[commit-bot: I haz the power, 2016-11-23 18:35:07]

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

[commit-bot: I haz the power, 2016-11-23 18:56:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 18:56:15]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[llozano, 2016-11-23 19:56:53]

On 2016/11/23 18:34:33, Sam Clegg wrote:
> PTAL.
> 
> Even though trybot is red, the compile phase is green and this bots test
phases
> having been green in forever.

I have no idea about the linux_arm failure. It is complaining about a missing
directory.

for linux_android_rel_eng, I see:

/b/c/cipd/goma/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++
-MMD -MF clang_x64/obj/base/allocator/tcmalloc/system-alloc.o.d -DNO_HEAP_CHECK
-DV8_DEPRECATION_WARNINGS -DENABLE_NOTIFICATIONS -DENABLE_PDF=1
-DDCHECK_ALWAYS_ON=1 -DUSE_UDEV -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1
-DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1
-DENABLE_TASK_MANAGER=1 -DENABLE_THEMES=1 -DUSE_PROPRIETARY_CODECS
-DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL
-DCHROMIUM_BUILD -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED
-DCR_CLANG_REVISION=287685-1 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
-D_LARGEFILE64_SOURCE -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0
-DTCMALLOC_DONT_REPLACE_SYSTEM_ALLOC -I../../base/allocator
-I../../third_party/tcmalloc/chromium/src/base
-I../../third_party/tcmalloc/chromium/src -I../.. -Iclang_x64/gen
-fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -funwind-tables
-fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin
-fcolor-diagnostics -fdebug-prefix-map=/b/c/b/android/src=. -pthread -m64
-march=x86-64 -g1 --sysroot=../../build/linux/debian_wheezy_amd64-sysroot
-fvisibility=hidden -Xclang -load -Xclang
../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang
-add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs
-Xclang check-ipc -Wheader-hygiene -Wstring-conversion -Werror -Wall
-Wno-unused-variable -Wno-missing-field-initializers -Wno-unused-parameter
-Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register
-Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override
-Wno-shift-negative-value -Wno-undefined-var-template
-Wno-nonportable-include-path -Wno-address-of-packed-member -Wno-reorder
-Wno-unused-function -Wno-unused-local-typedefs -Wno-unused-private-field -marm
-Wno-sign-compare -Wno-unused-result -O2 -fno-ident -fdata-sections
-ffunction-sections -fno-threadsafe-statics -fvisibility-inlines-hidden
-std=gnu++11 -fno-rtti -fno-exceptions -Wno-deprecated -c
../../third_party/tcmalloc/chromium/src/system-alloc.cc -o
clang_x64/obj/base/allocator/tcmalloc/system-alloc.o
clang: error: argument unused during compilation: '-mno-thumb'
[-Werror,-Wunused-command-line-argument]

this is very annoying.. 
I hate to suggest this but can you conditionally add -marm only for chromeos? 
(This is becoming a horrible hack)

[Sam Clegg, 2016-11-23 20:09:28]

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

[commit-bot: I haz the power, 2016-11-23 20:10:04]

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

[Sam Clegg, 2016-11-23 20:15:24]

On 2016/11/23 19:56:53, llozano wrote:
> On 2016/11/23 18:34:33, Sam Clegg wrote:
> > PTAL.
> > 
> > Even though trybot is red, the compile phase is green and this bots test
> phases
> > having been green in forever.
> 
> I have no idea about the linux_arm failure. It is complaining about a missing
> directory.
> 
> for linux_android_rel_eng, I see:
> 
> /b/c/cipd/goma/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++
> -MMD -MF clang_x64/obj/base/allocator/tcmalloc/system-alloc.o.d
-DNO_HEAP_CHECK
> -DV8_DEPRECATION_WARNINGS -DENABLE_NOTIFICATIONS -DENABLE_PDF=1
> -DDCHECK_ALWAYS_ON=1 -DUSE_UDEV -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1
> -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1
> -DENABLE_TASK_MANAGER=1 -DENABLE_THEMES=1 -DUSE_PROPRIETARY_CODECS
> -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL
> -DCHROMIUM_BUILD -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED
> -DCR_CLANG_REVISION=287685-1 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
> -D_LARGEFILE64_SOURCE -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0
> -DTCMALLOC_DONT_REPLACE_SYSTEM_ALLOC -I../../base/allocator
> -I../../third_party/tcmalloc/chromium/src/base
> -I../../third_party/tcmalloc/chromium/src -I../.. -Iclang_x64/gen
> -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector
-funwind-tables
> -fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin
> -fcolor-diagnostics -fdebug-prefix-map=/b/c/b/android/src=. -pthread -m64
> -march=x86-64 -g1 --sysroot=../../build/linux/debian_wheezy_amd64-sysroot
> -fvisibility=hidden -Xclang -load -Xclang
> ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
-Xclang
> -add-plugin -Xclang find-bad-constructs -Xclang
-plugin-arg-find-bad-constructs
> -Xclang check-ipc -Wheader-hygiene -Wstring-conversion -Werror -Wall
> -Wno-unused-variable -Wno-missing-field-initializers -Wno-unused-parameter
> -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register
> -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override
> -Wno-shift-negative-value -Wno-undefined-var-template
> -Wno-nonportable-include-path -Wno-address-of-packed-member -Wno-reorder
> -Wno-unused-function -Wno-unused-local-typedefs -Wno-unused-private-field
-marm
> -Wno-sign-compare -Wno-unused-result -O2 -fno-ident -fdata-sections
> -ffunction-sections -fno-threadsafe-statics -fvisibility-inlines-hidden
> -std=gnu++11 -fno-rtti -fno-exceptions -Wno-deprecated -c
> ../../third_party/tcmalloc/chromium/src/system-alloc.cc -o
> clang_x64/obj/base/allocator/tcmalloc/system-alloc.o
> clang: error: argument unused during compilation: '-mno-thumb'
> [-Werror,-Wunused-command-line-argument]
> 
> this is very annoying.. 
> I hate to suggest this but can you conditionally add -marm only for chromeos? 
> (This is becoming a horrible hack)

Ack. Thats too bad.

I added is_chromeos and a comment.

In the long run I really think that the clang wrapper on chromeos should not be
setting -mthumb.   I looked at clang and it always defaults to arm mode (there
is no way to configure it to use thumb by default) so I think its reasonable to
gn to assume that clang builds are arm-mode-by-default unless we pass -mthumb.

See:
https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains.cpp#L2570

The arm_use_thumb option already default to true.   Is there some way the
compiler wrapper be configured not to pass -mthumb when building packages that
already have this option (i.e. chromium)?

[llozano, 2016-11-23 20:40:01]

On 2016/11/23 20:15:24, Sam Clegg wrote:
> On 2016/11/23 19:56:53, llozano wrote:
> > On 2016/11/23 18:34:33, Sam Clegg wrote:
> > > PTAL.
> > > 
> > > Even though trybot is red, the compile phase is green and this bots test
> > phases
> > > having been green in forever.
> > 
> > I have no idea about the linux_arm failure. It is complaining about a
missing
> > directory.
> > 
> > for linux_android_rel_eng, I see:
> > 
> > /b/c/cipd/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++
> > -MMD -MF clang_x64/obj/base/allocator/tcmalloc/system-alloc.o.d
> -DNO_HEAP_CHECK
> > -DV8_DEPRECATION_WARNINGS -DENABLE_NOTIFICATIONS -DENABLE_PDF=1
> > -DDCHECK_ALWAYS_ON=1 -DUSE_UDEV -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1
> > -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1
> > -DENABLE_TASK_MANAGER=1 -DENABLE_THEMES=1 -DUSE_PROPRIETARY_CODECS
> > -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL
> > -DCHROMIUM_BUILD -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED
> > -DCR_CLANG_REVISION=287685-1 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
> > -D_LARGEFILE64_SOURCE -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0
> > -DTCMALLOC_DONT_REPLACE_SYSTEM_ALLOC -I../../base/allocator
> > -I../../third_party/tcmalloc/chromium/src/base
> > -I../../third_party/tcmalloc/chromium/src -I../.. -Iclang_x64/gen
> > -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector
> -funwind-tables
> > -fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin
> > -fcolor-diagnostics -fdebug-prefix-map=/b/c/b/android/src=. -pthread -m64
> > -march=x86-64 -g1 --sysroot=../../build/linux/debian_wheezy_amd64-sysroot
> > -fvisibility=hidden -Xclang -load -Xclang
> > ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
> -Xclang
> > -add-plugin -Xclang find-bad-constructs -Xclang
> -plugin-arg-find-bad-constructs
> > -Xclang check-ipc -Wheader-hygiene -Wstring-conversion -Werror -Wall
> > -Wno-unused-variable -Wno-missing-field-initializers -Wno-unused-parameter
> > -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register
> > -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override
> > -Wno-shift-negative-value -Wno-undefined-var-template
> > -Wno-nonportable-include-path -Wno-address-of-packed-member -Wno-reorder
> > -Wno-unused-function -Wno-unused-local-typedefs -Wno-unused-private-field
> -marm
> > -Wno-sign-compare -Wno-unused-result -O2 -fno-ident -fdata-sections
> > -ffunction-sections -fno-threadsafe-statics -fvisibility-inlines-hidden
> > -std=gnu++11 -fno-rtti -fno-exceptions -Wno-deprecated -c
> > ../../third_party/tcmalloc/chromium/src/system-alloc.cc -o
> > clang_x64/obj/base/allocator/tcmalloc/system-alloc.o
> > clang: error: argument unused during compilation: '-mno-thumb'
> > [-Werror,-Wunused-command-line-argument]
> > 
> > this is very annoying.. 
> > I hate to suggest this but can you conditionally add -marm only for
chromeos? 
> > (This is becoming a horrible hack)
> 
> Ack. Thats too bad.
> 
> I added is_chromeos and a comment.
> 
> In the long run I really think that the clang wrapper on chromeos should not
be
> setting -mthumb.   I looked at clang and it always defaults to arm mode (there
> is no way to configure it to use thumb by default) so I think its reasonable
to
> gn to assume that clang builds are arm-mode-by-default unless we pass -mthumb.
> 
> See:
>
https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains.cpp#L2570
> 
> The arm_use_thumb option already default to true.   Is there some way the
> compiler wrapper be configured not to pass -mthumb when building packages that
> already have this option (i.e. chromium)?

It seems to be there is a way to "configure" clang to use thumb by changing the
arch value in the tripple. 
And maybe that is what chromeos should be doing. 
But this does not solve this problem. GN would still have to say to ChromeOS it
needs -marm for tcmalloc. And it can only say to ChromeOS since that that is the
only place where the default is "thumb".
And there should not be any issue in ChromeOS wanting to build everything with
thumb.

Lets see if this last version of this CL works.

[Sam Clegg, 2016-11-23 21:26:00]

On 2016/11/23 20:40:01, llozano wrote:
> On 2016/11/23 20:15:24, Sam Clegg wrote:
> > On 2016/11/23 19:56:53, llozano wrote:
> > > On 2016/11/23 18:34:33, Sam Clegg wrote:
> > > > PTAL.
> > > > 
> > > > Even though trybot is red, the compile phase is green and this bots test
> > > phases
> > > > having been green in forever.
> > > 
> > > I have no idea about the linux_arm failure. It is complaining about a
> missing
> > > directory.
> > > 
> > > for linux_android_rel_eng, I see:
> > > 
> > > /b/c/cipd/goma/gomacc
> ../../third_party/llvm-build/Release+Asserts/bin/clang++
> > > -MMD -MF clang_x64/obj/base/allocator/tcmalloc/system-alloc.o.d
> > -DNO_HEAP_CHECK
> > > -DV8_DEPRECATION_WARNINGS -DENABLE_NOTIFICATIONS -DENABLE_PDF=1
> > > -DDCHECK_ALWAYS_ON=1 -DUSE_UDEV -DUI_COMPOSITOR_IMAGE_TRANSPORT
-DUSE_AURA=1
> > > -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1
> > > -DENABLE_TASK_MANAGER=1 -DENABLE_THEMES=1 -DUSE_PROPRIETARY_CODECS
> > > -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL
> > > -DCHROMIUM_BUILD -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED
> > > -DCR_CLANG_REVISION=287685-1 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
> > > -D_LARGEFILE64_SOURCE -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0
> > > -DTCMALLOC_DONT_REPLACE_SYSTEM_ALLOC -I../../base/allocator
> > > -I../../third_party/tcmalloc/chromium/src/base
> > > -I../../third_party/tcmalloc/chromium/src -I../.. -Iclang_x64/gen
> > > -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector
> > -funwind-tables
> > > -fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin
> > > -fcolor-diagnostics -fdebug-prefix-map=/b/c/b/android/src=. -pthread -m64
> > > -march=x86-64 -g1 --sysroot=../../build/linux/debian_wheezy_amd64-sysroot
> > > -fvisibility=hidden -Xclang -load -Xclang
> > > ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
> > -Xclang
> > > -add-plugin -Xclang find-bad-constructs -Xclang
> > -plugin-arg-find-bad-constructs
> > > -Xclang check-ipc -Wheader-hygiene -Wstring-conversion -Werror -Wall
> > > -Wno-unused-variable -Wno-missing-field-initializers -Wno-unused-parameter
> > > -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register
> > > -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override
> > > -Wno-shift-negative-value -Wno-undefined-var-template
> > > -Wno-nonportable-include-path -Wno-address-of-packed-member -Wno-reorder
> > > -Wno-unused-function -Wno-unused-local-typedefs -Wno-unused-private-field
> > -marm
> > > -Wno-sign-compare -Wno-unused-result -O2 -fno-ident -fdata-sections
> > > -ffunction-sections -fno-threadsafe-statics -fvisibility-inlines-hidden
> > > -std=gnu++11 -fno-rtti -fno-exceptions -Wno-deprecated -c
> > > ../../third_party/tcmalloc/chromium/src/system-alloc.cc -o
> > > clang_x64/obj/base/allocator/tcmalloc/system-alloc.o
> > > clang: error: argument unused during compilation: '-mno-thumb'
> > > [-Werror,-Wunused-command-line-argument]
> > > 
> > > this is very annoying.. 
> > > I hate to suggest this but can you conditionally add -marm only for
> chromeos? 
> > > (This is becoming a horrible hack)
> > 
> > Ack. Thats too bad.
> > 
> > I added is_chromeos and a comment.
> > 
> > In the long run I really think that the clang wrapper on chromeos should not
> be
> > setting -mthumb.   I looked at clang and it always defaults to arm mode
(there
> > is no way to configure it to use thumb by default) so I think its reasonable
> to
> > gn to assume that clang builds are arm-mode-by-default unless we pass
-mthumb.
> > 
> > See:
> >
>
https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains.cpp#L2570
> > 
> > The arm_use_thumb option already default to true.   Is there some way the
> > compiler wrapper be configured not to pass -mthumb when building packages
that
> > already have this option (i.e. chromium)?
> 
> It seems to be there is a way to "configure" clang to use thumb by changing
the
> arch value in the tripple. 
> And maybe that is what chromeos should be doing. 
> But this does not solve this problem. GN would still have to say to ChromeOS
it
> needs -marm for tcmalloc. And it can only say to ChromeOS since that that is
the
> only place where the default is "thumb".
> And there should not be any issue in ChromeOS wanting to build everything with
> thumb.

But there *is* an issue with way chromeos is trying to build with thumb.  It
should
be using the gn option, not creating a compiler wrapper script that contradicts
the expectations that developers have have about the behavior of clang.

If one wants to build chrome with certain flags the I would argue that the
correct
way to do that is to use the gn build system specify the options, not put the
options in a wrapper script.. otherwise we end up in situations like this.

Anyway, I'm happy to land this patch in the interim.


> 
> Lets see if this last version of this CL works.

[Sam Clegg, 2016-11-23 21:28:32]

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

[commit-bot: I haz the power, 2016-11-23 21:29:05]

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

[commit-bot: I haz the power, 2016-11-24 01:05:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 01:05:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Sam Clegg, 2016-11-28 19:58:41]

On 2016/11/23 21:26:00, Sam Clegg wrote:
> On 2016/11/23 20:40:01, llozano wrote:
> > On 2016/11/23 20:15:24, Sam Clegg wrote:
> > > On 2016/11/23 19:56:53, llozano wrote:
> > > > On 2016/11/23 18:34:33, Sam Clegg wrote:
> > > > > PTAL.
> > > > > 
> > > > > Even though trybot is red, the compile phase is green and this bots
test
> > > > phases
> > > > > having been green in forever.
> > > > 
> > > > I have no idea about the linux_arm failure. It is complaining about a
> > missing
> > > > directory.
> > > > 
> > > > for linux_android_rel_eng, I see:
> > > > 
> > > > /b/c/cipd/goma/gomacc
> > ../../third_party/llvm-build/Release+Asserts/bin/clang++
> > > > -MMD -MF clang_x64/obj/base/allocator/tcmalloc/system-alloc.o.d
> > > -DNO_HEAP_CHECK
> > > > -DV8_DEPRECATION_WARNINGS -DENABLE_NOTIFICATIONS -DENABLE_PDF=1
> > > > -DDCHECK_ALWAYS_ON=1 -DUSE_UDEV -DUI_COMPOSITOR_IMAGE_TRANSPORT
> -DUSE_AURA=1
> > > > -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1
> > > > -DENABLE_TASK_MANAGER=1 -DENABLE_THEMES=1 -DUSE_PROPRIETARY_CODECS
> > > > -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL
> > > > -DCHROMIUM_BUILD -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED
> > > > -DCR_CLANG_REVISION=287685-1 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
> > > > -D_LARGEFILE64_SOURCE -DNDEBUG -DNVALGRIND
-DDYNAMIC_ANNOTATIONS_ENABLED=0
> > > > -DTCMALLOC_DONT_REPLACE_SYSTEM_ALLOC -I../../base/allocator
> > > > -I../../third_party/tcmalloc/chromium/src/base
> > > > -I../../third_party/tcmalloc/chromium/src -I../.. -Iclang_x64/gen
> > > > -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector
> > > -funwind-tables
> > > > -fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin
> > > > -fcolor-diagnostics -fdebug-prefix-map=/b/c/b/android/src=. -pthread
-m64
> > > > -march=x86-64 -g1
--sysroot=../../build/linux/debian_wheezy_amd64-sysroot
> > > > -fvisibility=hidden -Xclang -load -Xclang
> > > > ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
> > > -Xclang
> > > > -add-plugin -Xclang find-bad-constructs -Xclang
> > > -plugin-arg-find-bad-constructs
> > > > -Xclang check-ipc -Wheader-hygiene -Wstring-conversion -Werror -Wall
> > > > -Wno-unused-variable -Wno-missing-field-initializers
-Wno-unused-parameter
> > > > -Wno-c++11-narrowing -Wno-covered-switch-default
-Wno-deprecated-register
> > > > -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override
> > > > -Wno-shift-negative-value -Wno-undefined-var-template
> > > > -Wno-nonportable-include-path -Wno-address-of-packed-member -Wno-reorder
> > > > -Wno-unused-function -Wno-unused-local-typedefs
-Wno-unused-private-field
> > > -marm
> > > > -Wno-sign-compare -Wno-unused-result -O2 -fno-ident -fdata-sections
> > > > -ffunction-sections -fno-threadsafe-statics -fvisibility-inlines-hidden
> > > > -std=gnu++11 -fno-rtti -fno-exceptions -Wno-deprecated -c
> > > > ../../third_party/tcmalloc/chromium/src/system-alloc.cc -o
> > > > clang_x64/obj/base/allocator/tcmalloc/system-alloc.o
> > > > clang: error: argument unused during compilation: '-mno-thumb'
> > > > [-Werror,-Wunused-command-line-argument]
> > > > 
> > > > this is very annoying.. 
> > > > I hate to suggest this but can you conditionally add -marm only for
> > chromeos? 
> > > > (This is becoming a horrible hack)
> > > 
> > > Ack. Thats too bad.
> > > 
> > > I added is_chromeos and a comment.
> > > 
> > > In the long run I really think that the clang wrapper on chromeos should
not
> > be
> > > setting -mthumb.   I looked at clang and it always defaults to arm mode
> (there
> > > is no way to configure it to use thumb by default) so I think its
reasonable
> > to
> > > gn to assume that clang builds are arm-mode-by-default unless we pass
> -mthumb.
> > > 
> > > See:
> > >
> >
>
https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains.cpp#L2570
> > > 
> > > The arm_use_thumb option already default to true.   Is there some way the
> > > compiler wrapper be configured not to pass -mthumb when building packages
> that
> > > already have this option (i.e. chromium)?
> > 
> > It seems to be there is a way to "configure" clang to use thumb by changing
> the
> > arch value in the tripple. 
> > And maybe that is what chromeos should be doing. 
> > But this does not solve this problem. GN would still have to say to ChromeOS
> it
> > needs -marm for tcmalloc. And it can only say to ChromeOS since that that is
> the
> > only place where the default is "thumb".
> > And there should not be any issue in ChromeOS wanting to build everything
with
> > thumb.
> 
> But there *is* an issue with way chromeos is trying to build with thumb.  It
> should
> be using the gn option, not creating a compiler wrapper script that
contradicts
> the expectations that developers have have about the behavior of clang.
> 
> If one wants to build chrome with certain flags the I would argue that the
> correct
> way to do that is to use the gn build system specify the options, not put the
> options in a wrapper script.. otherwise we end up in situations like this.
> 
> Anyway, I'm happy to land this patch in the interim.
> 
> 
> > 
> > Lets see if this last version of this CL works.

Are we in agreement that this is a good interim solution?

[llozano1, 2016-11-28 20:33:26]

On 2016/11/28 19:58:41, Sam Clegg wrote:
> On 2016/11/23 21:26:00, Sam Clegg wrote:
> > On 2016/11/23 20:40:01, llozano wrote:
> > > On 2016/11/23 20:15:24, Sam Clegg wrote:
> > > > On 2016/11/23 19:56:53, llozano wrote:
> > > > > On 2016/11/23 18:34:33, Sam Clegg wrote:
> > > > > > PTAL.
> > > > > > 
> > > > > > Even though trybot is red, the compile phase is green and this bots
> test
> > > > > phases
> > > > > > having been green in forever.
> > > > > 
> > > > > I have no idea about the linux_arm failure. It is complaining about a
> > > missing
> > > > > directory.
> > > > > 
> > > > > for linux_android_rel_eng, I see:
> > > > > 
> > > > > /b/c/cipd/goma/gomacc
> > > ../../third_party/llvm-build/Release+Asserts/bin/clang++
> > > > > -MMD -MF clang_x64/obj/base/allocator/tcmalloc/system-alloc.o.d
> > > > -DNO_HEAP_CHECK
> > > > > -DV8_DEPRECATION_WARNINGS -DENABLE_NOTIFICATIONS -DENABLE_PDF=1
> > > > > -DDCHECK_ALWAYS_ON=1 -DUSE_UDEV -DUI_COMPOSITOR_IMAGE_TRANSPORT
> > -DUSE_AURA=1
> > > > > -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1
> > > > > -DENABLE_TASK_MANAGER=1 -DENABLE_THEMES=1 -DUSE_PROPRIETARY_CODECS
> > > > > -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL
> > > > > -DCHROMIUM_BUILD -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED
> > > > > -DCR_CLANG_REVISION=287685-1 -D_FILE_OFFSET_BITS=64
-D_LARGEFILE_SOURCE
> > > > > -D_LARGEFILE64_SOURCE -DNDEBUG -DNVALGRIND
> -DDYNAMIC_ANNOTATIONS_ENABLED=0
> > > > > -DTCMALLOC_DONT_REPLACE_SYSTEM_ALLOC -I../../base/allocator
> > > > > -I../../third_party/tcmalloc/chromium/src/base
> > > > > -I../../third_party/tcmalloc/chromium/src -I../.. -Iclang_x64/gen
> > > > > -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector
> > > > -funwind-tables
> > > > > -fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin
> > > > > -fcolor-diagnostics -fdebug-prefix-map=/b/c/b/android/src=. -pthread
> -m64
> > > > > -march=x86-64 -g1
> --sysroot=../../build/linux/debian_wheezy_amd64-sysroot
> > > > > -fvisibility=hidden -Xclang -load -Xclang
> > > > >
../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
> > > > -Xclang
> > > > > -add-plugin -Xclang find-bad-constructs -Xclang
> > > > -plugin-arg-find-bad-constructs
> > > > > -Xclang check-ipc -Wheader-hygiene -Wstring-conversion -Werror -Wall
> > > > > -Wno-unused-variable -Wno-missing-field-initializers
> -Wno-unused-parameter
> > > > > -Wno-c++11-narrowing -Wno-covered-switch-default
> -Wno-deprecated-register
> > > > > -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override
> > > > > -Wno-shift-negative-value -Wno-undefined-var-template
> > > > > -Wno-nonportable-include-path -Wno-address-of-packed-member
-Wno-reorder
> > > > > -Wno-unused-function -Wno-unused-local-typedefs
> -Wno-unused-private-field
> > > > -marm
> > > > > -Wno-sign-compare -Wno-unused-result -O2 -fno-ident -fdata-sections
> > > > > -ffunction-sections -fno-threadsafe-statics
-fvisibility-inlines-hidden
> > > > > -std=gnu++11 -fno-rtti -fno-exceptions -Wno-deprecated -c
> > > > > ../../third_party/tcmalloc/chromium/src/system-alloc.cc -o
> > > > > clang_x64/obj/base/allocator/tcmalloc/system-alloc.o
> > > > > clang: error: argument unused during compilation: '-mno-thumb'
> > > > > [-Werror,-Wunused-command-line-argument]
> > > > > 
> > > > > this is very annoying.. 
> > > > > I hate to suggest this but can you conditionally add -marm only for
> > > chromeos? 
> > > > > (This is becoming a horrible hack)
> > > > 
> > > > Ack. Thats too bad.
> > > > 
> > > > I added is_chromeos and a comment.
> > > > 
> > > > In the long run I really think that the clang wrapper on chromeos should
> not
> > > be
> > > > setting -mthumb.   I looked at clang and it always defaults to arm mode
> > (there
> > > > is no way to configure it to use thumb by default) so I think its
> reasonable
> > > to
> > > > gn to assume that clang builds are arm-mode-by-default unless we pass
> > -mthumb.
> > > > 
> > > > See:
> > > >
> > >
> >
>
https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains.cpp#L2570
> > > > 
> > > > The arm_use_thumb option already default to true.   Is there some way
the
> > > > compiler wrapper be configured not to pass -mthumb when building
packages
> > that
> > > > already have this option (i.e. chromium)?
> > > 
> > > It seems to be there is a way to "configure" clang to use thumb by
changing
> > the
> > > arch value in the tripple. 
> > > And maybe that is what chromeos should be doing. 
> > > But this does not solve this problem. GN would still have to say to
ChromeOS
> > it
> > > needs -marm for tcmalloc. And it can only say to ChromeOS since that that
is
> > the
> > > only place where the default is "thumb".
> > > And there should not be any issue in ChromeOS wanting to build everything
> with
> > > thumb.
> > 
> > But there *is* an issue with way chromeos is trying to build with thumb.  It
> > should
> > be using the gn option, not creating a compiler wrapper script that
> contradicts
> > the expectations that developers have have about the behavior of clang.
> > 
> > If one wants to build chrome with certain flags the I would argue that the
> > correct
> > way to do that is to use the gn build system specify the options, not put
the
> > options in a wrapper script.. otherwise we end up in situations like this.
> > 
> > Anyway, I'm happy to land this patch in the interim.
> > 
> > 
> > > 
> > > Lets see if this last version of this CL works.
> 
> Are we in agreement that this is a good interim solution?

works on ChromeOS (just verified it)

[Dirk Pranke, 2016-11-29 00:24:24]

Can we do this via configs instead of just adding the flag? I don't want GN to
be passing both -m thumb and -m arm and relying on the ordering of flags. I
don't want the CrOS compiler passing -m thumb in the wrapper, either, but that's
a separate issue. We should make GN do the right thing regardless.

[Sam Clegg, 2016-11-29 00:33:17]

On 2016/11/29 00:24:24, Dirk Pranke wrote:
> Can we do this via configs instead of just adding the flag? I don't want GN to
> be passing both -m thumb and -m arm and relying on the ordering of flags. I
> don't want the CrOS compiler passing -m thumb in the wrapper, either, but
that's
> a separate issue. We should make GN do the right thing regardless.

You mean like adding a new `compiler_arm_force` which is would only apply on
chromeos?

GN itself should never been passing both, even with this change.

[Dirk Pranke, 2016-11-29 01:13:20]

On 2016/11/29 00:33:17, Sam Clegg wrote:
> On 2016/11/29 00:24:24, Dirk Pranke wrote:
> > Can we do this via configs instead of just adding the flag? I don't want GN
to
> > be passing both -m thumb and -m arm and relying on the ordering of flags. I
> > don't want the CrOS compiler passing -m thumb in the wrapper, either, but
> that's
> > a separate issue. We should make GN do the right thing regardless.
> 
> You mean like adding a new `compiler_arm_force` which is would only apply on
> chromeos?
> 
> GN itself should never been passing both, even with this change.

Ah, sorry, I lost track of what landed when.

I see now that we're removing the :compiler_arm_thumb config, and so we won't
pass -mthumb
and this patch would work.

But, yes, ideally we'd have ":compiler_arm" config that we'd add, rather than
adding the flag directly,
and then we can put the two workarounds together in the file.

[Sam Clegg, 2016-11-30 22:04:44]

On 2016/11/29 01:13:20, Dirk Pranke wrote:
> On 2016/11/29 00:33:17, Sam Clegg wrote:
> > On 2016/11/29 00:24:24, Dirk Pranke wrote:
> > > Can we do this via configs instead of just adding the flag? I don't want
GN
> to
> > > be passing both -m thumb and -m arm and relying on the ordering of flags.
I
> > > don't want the CrOS compiler passing -m thumb in the wrapper, either, but
> > that's
> > > a separate issue. We should make GN do the right thing regardless.
> > 
> > You mean like adding a new `compiler_arm_force` which is would only apply on
> > chromeos?
> > 
> > GN itself should never been passing both, even with this change.
> 
> Ah, sorry, I lost track of what landed when.
> 
> I see now that we're removing the :compiler_arm_thumb config, and so we won't
> pass -mthumb
> and this patch would work.
> 
> But, yes, ideally we'd have ":compiler_arm" config that we'd add, rather than
> adding the flag directly,
> and then we can put the two workarounds together in the file.

Done

[Dirk Pranke, 2016-11-30 23:25:13]

lgtm now, thanks!

[Sam Clegg, 2016-11-30 23:48:46]

The CQ bit was checked by sbc@chromium.org

[commit-bot: I haz the power, 2016-11-30 23:49:24]

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

[commit-bot: I haz the power, 2016-11-30 23:56:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 23:56:33]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Sam Clegg, 2016-12-01 00:00:55]

brettw, PTAL

[Nico, 2016-12-02 01:52:03]

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

[Nico, 2016-12-02 01:52:04]

I think brettw is out this week, but base/ lgtm (+primiano fyi since it's
allocator)

[Nico, 2016-12-02 01:52:08]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2016-12-02 01:52:40]

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

[commit-bot: I haz the power, 2016-12-02 05:11:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 05:11:14]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Nico, 2016-12-02 11:15:59]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2016-12-02 11:16:13]

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

[commit-bot: I haz the power, 2016-12-02 12:14:35]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480677359343350,
"parent_rev": "d907de4f50ebaf915e805b86d409a945f3a133c6", "commit_rev":
"8a093dc0b8f4d36123ee457c882291692a97d216"}

[commit-bot: I haz the power, 2016-12-02 12:15:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-02 12:16:21]

Description was changed from

==========
Explictly pass -marm when arm mode is required

Simply disabling the -mthumb flag is not enough
because the chromeos toolchain defaults to thumb.

BUG=564059
==========

to

==========
Explictly pass -marm when arm mode is required

Simply disabling the -mthumb flag is not enough
because the chromeos toolchain defaults to thumb.

BUG=564059

Committed: https://crrev.com/c0e12589bf7ae45f0f463d4726224442af90e142
Cr-Commit-Position: refs/heads/master@{#435919}
==========

[commit-bot: I haz the power, 2016-12-02 12:16:23]

Patchset 4 (id:??) landed as
https://crrev.com/c0e12589bf7ae45f0f463d4726224442af90e142
Cr-Commit-Position: refs/heads/master@{#435919}