Chrome OS: Reduce NOTREACHED() overhead for release builds.

Chrome OS: Reduce NOTREACHED() overhead for release builds.

For Chrome OS release builds, implement a dedicated LogErrorNotReached() function to reduce
the number of function calls per NOTREACHED() call site from 5+ to 1 and drop custom messages that are appended to NOTREACHED().  (Release builds on other platforms drop NOTREACHED() altogether, so no changes there.)

This change reduces the size of my chromeos=1 executable by 1.7 MB (ca. 1%).

BUG=484283
Committed: https://crrev.com/ff3f34ae4fefa2d0ecce5ba338b1afb9505c72d4
Cr-Commit-Position: refs/heads/master@{#331249}

[Thiemo Nagel, 2015-05-04 17:59:04]

tnagel@chromium.org changed reviewers:
+ danakj@chromium.org

[Thiemo Nagel, 2015-05-04 17:59:05]

Hi Dana,

may I ask you to take a look at this improvement to the logging code?

Thank you!
Thiemo

[danakj, 2015-05-04 18:21:46]

https://codereview.chromium.org/1124673004/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1124673004/diff/1/base/logging.h#newcode657
base/logging.h:657: LAZY_STREAM(LOG_STREAM(DCHECK), DCHECK_IS_ON() &&
!(condition)) \
This is unrelated, please don't lump multiple logical changes into one CL.

[danakj, 2015-05-04 18:27:41]

https://codereview.chromium.org/1124673004/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1124673004/diff/1/base/logging.h#newcode707
base/logging.h:707: true ? ::logging::LogErrorNotReached(__FILE__, __LINE__,
__FUNCTION__) \
what's the point of the ternary that's always true?

[Thiemo Nagel, 2015-05-04 19:15:47]

Thank you for the review!  Could you please take another look?

https://codereview.chromium.org/1124673004/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1124673004/diff/1/base/logging.h#newcode657
base/logging.h:657: LAZY_STREAM(LOG_STREAM(DCHECK), DCHECK_IS_ON() &&
!(condition)) \
On 2015/05/04 18:21:46, danakj wrote:
> This is unrelated, please don't lump multiple logical changes into one CL.

Done.

https://codereview.chromium.org/1124673004/diff/1/base/logging.h#newcode707
base/logging.h:707: true ? ::logging::LogErrorNotReached(__FILE__, __LINE__,
__FUNCTION__) \
On 2015/05/04 18:27:41, danakj wrote:
> what's the point of the ternary that's always true?

The ternary is used as a binder so that EAT_STREAM_PARAMETERS can be attached as
part of the same, single statement.  Using two statements would break use cases
such as "if (cond) NOTREACHED(); else blah();".

[Thiemo Nagel, 2015-05-18 14:30:24]

Hi Dana,

may I send a friendly ping about this?

Thank you,
Thiemo

[danakj, 2015-05-22 20:29:16]

LGTM

[Thiemo Nagel, 2015-05-24 07:34:20]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2015-05-24 07:34:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124673004/40001

[Thiemo Nagel, 2015-05-24 07:34:30]

On 2015/05/22 20:29:16, danakj wrote:
> LGTM

Thank you!

[commit-bot: I haz the power, 2015-05-24 12:59:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-24 13:00:27]

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

[Shu Chen, 2015-05-25 08:29:53]

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

[Shu Chen, 2015-05-25 08:29:54]

This cl seems causing compiling failures to my local sandbox chrome os build.

[danakj, 2015-05-25 17:21:18]

it passed the cq. what's the error?
On May 25, 2015 1:29 AM, <shuchen@chromium.org> wrote:

> This cl seems causing compiling failures to my local sandbox chrome os
> build.
>
>
> https://codereview.chromium.org/1124673004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Shu Chen, 2015-05-26 02:57:54]

On 2015/05/25 17:21:18, danakj wrote:
> it passed the cq. what's the error?
> On May 25, 2015 1:29 AM, <mailto:shuchen@chromium.org> wrote:
> 
> > This cl seems causing compiling failures to my local sandbox chrome os
> > build.
> >
> >
> > https://codereview.chromium.org/1124673004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Below is the error:

shuchen@chenshu:~/chrome/src$ ninja -j 50 -C out_cros/Release chrome 
ninja: Entering directory `out_cros/Release'
[53/9489] SOLINK lib/libcrcrypto.so
FAILED: if [ ! -e lib/libcrcrypto.so -o ! -e lib/libcrcrypto.so.TOC ]; then
../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
-Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,defs -pthread -Wl,-z,noexecstack -fPIC
-fuse-ld=gold
-B/usr/local/google/home/shuchen/chrome/src/third_party/binutils/Linux_x64/Release/bin
-Wl,--disable-new-dtags -m64 -Wl,--icf=safe -Wl,-O1 -Wl,--as-needed
-Wl,--gc-sections -o lib/libcrcrypto.so -Wl,-soname=libcrcrypto.so
@lib/libcrcrypto.so.rsp && { readelf -d lib/libcrcrypto.so | grep SONAME ; nm
-gD -f p lib/libcrcrypto.so | cut -f1-2 -d' '; } > lib/libcrcrypto.so.TOC; else
../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
-Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,defs -pthread -Wl,-z,noexecstack -fPIC
-fuse-ld=gold
-B/usr/local/google/home/shuchen/chrome/src/third_party/binutils/Linux_x64/Release/bin
-Wl,--disable-new-dtags -m64 -Wl,--icf=safe -Wl,-O1 -Wl,--as-needed
-Wl,--gc-sections -o lib/libcrcrypto.so -Wl,-soname=libcrcrypto.so
@lib/libcrcrypto.so.rsp && { readelf -d lib/libcrcrypto.so | grep SONAME ; nm
-gD -f p lib/libcrcrypto.so | cut -f1-2 -d' '; } > lib/libcrcrypto.so.tmp && if
! cmp -s lib/libcrcrypto.so.tmp lib/libcrcrypto.so.TOC; then mv
lib/libcrcrypto.so.tmp lib/libcrcrypto.so.TOC ; fi; fi                          
                                                                                
           ../../crypto/hmac.cc:31: error: undefined reference to
'logging::LogErrorNotReached(char const*, int)'
../../crypto/hmac.cc:31: error: undefined reference to
'logging::LogErrorNotReached(char const*, int)'
../../crypto/hmac.cc:31: error: undefined reference to
'logging::LogErrorNotReached(char const*, int)'
../../crypto/encryptor_nss.cc:27: error: undefined reference to
'logging::LogErrorNotReached(char const*, int)'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
[53/9489] SOLINK lib/libevents_platform.so
FAILED: if [ ! -e lib/libevents_platform.so -o ! -e
lib/libevents_platform.so.TOC ]; then
../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
-Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,defs -pthread -Wl,-z,noexecstack -fPIC
-fuse-ld=gold
-B/usr/local/google/home/shuchen/chrome/src/third_party/binutils/Linux_x64/Release/bin
-Wl,--disable-new-dtags -m64 -Wl,--icf=safe -Wl,-O1 -Wl,--as-needed
-Wl,--gc-sections -o lib/libevents_platform.so -Wl,-soname=libevents_platform.so
@lib/libevents_platform.so.rsp && { readelf -d lib/libevents_platform.so | grep
SONAME ; nm -gD -f p lib/libevents_platform.so | cut -f1-2 -d' '; } >
lib/libevents_platform.so.TOC; else
../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
-Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,defs -pthread -Wl,-z,noexecstack -fPIC
-fuse-ld=gold
-B/usr/local/google/home/shuchen/chrome/src/third_party/binutils/Linux_x64/Release/bin
-Wl,--disable-new-dtags -m64 -Wl,--icf=safe -Wl,-O1 -Wl,--as-needed
-Wl,--gc-sections -o lib/libevents_platform.so -Wl,-soname=libevents_platform.so
@lib/libevents_platform.so.rsp && { readelf -d lib/libevents_platform.so | grep
SONAME ; nm -gD -f p lib/libevents_platform.so | cut -f1-2 -d' '; } >
lib/libevents_platform.so.tmp && if ! cmp -s lib/libevents_platform.so.tmp
lib/libevents_platform.so.TOC; then mv lib/libevents_platform.so.tmp
lib/libevents_platform.so.TOC ; fi; fi
../../base/observer_list.h:160: error: undefined reference to
'logging::LogErrorNotReached(char const*, int)'
../../base/observer_list.h:160: error: undefined reference to
'logging::LogErrorNotReached(char const*, int)'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
[53/9489] SOLINK lib/libbase_prefs.so
FAILED: if [ ! -e lib/libbase_prefs.so -o ! -e lib/libbase_prefs.so.TOC ]; then
../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
-Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,defs -pthread -Wl,-z,noexecstack -fPIC
-fuse-ld=gold
-B/usr/local/google/home/shuchen/chrome/src/third_party/binutils/Linux_x64/Release/bin
-Wl,--disable-new-dtags -m64 -Wl,--icf=safe -Wl,-O1 -Wl,--as-needed
-Wl,--gc-sections -o lib/libbase_prefs.so -Wl,-soname=libbase_prefs.so
@lib/libbase_prefs.so.rsp && { readelf -d lib/libbase_prefs.so | grep SONAME ;
nm -gD -f p lib/libbase_prefs.so | cut -f1-2 -d' '; } >
lib/libbase_prefs.so.TOC; else
../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
-Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,defs -pthread -Wl,-z,noexecstack -fPIC
-fuse-ld=gold
-B/usr/local/google/home/shuchen/chrome/src/third_party/binutils/Linux_x64/Release/bin
-Wl,--disable-new-dtags -m64 -Wl,--icf=safe -Wl,-O1 -Wl,--as-needed
-Wl,--gc-sections -o lib/libbase_prefs.so -Wl,-soname=libbase_prefs.so
@lib/libbase_prefs.so.rsp && { readelf -d lib/libbase_prefs.so | grep SONAME ;
nm -gD -f p lib/libbase_prefs.so | cut -f1-2 -d' '; } > lib/libbase_prefs.so.tmp
&& if ! cmp -s lib/libbase_prefs.so.tmp lib/libbase_prefs.so.TOC; then mv
lib/libbase_prefs.so.tmp lib/libbase_prefs.so.TOC ; fi; fi
../../base/observer_list.h:160: error: undefined reference to
'logging::LogErrorNotReached(char const*, int)'
../../base/prefs/json_pref_store.cc:373: error: undefined reference to
'logging::LogErrorNotReached(char const*, int)'
....

[Thiemo Nagel, 2015-05-26 09:05:57]

On May 25, 2015 1:29 AM, <mailto:shuchen@chromium.org> wrote:
> This cl seems causing compiling failures to my local sandbox chrome os
> build.

Thanks for the notification and sorry for the breakage!  :(

The problem was due to a missing BASE_EXPORT.  I have fixed it in
https://codereview.chromium.org/1155163002/.

[Nico, 2016-05-27 13:32:55]

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

[Nico, 2016-05-27 13:32:55]

Why is this chromeos-specific? It doesn't make sense to me to have
platform-specific code in logging.h.

[Thiemo Nagel, 2016-05-30 09:38:32]

On 2016/05/27 13:32:55, Nico (away until Tue May 31) wrote:
> Why is this chromeos-specific? It doesn't make sense to me to have
> platform-specific code in logging.h.

Chrome OS is different from all other platforms in that hitting NOTREACHED()
logs an error message.  I believe that the extra code size was deemed acceptable
for Chrome OS but not for other platforms.

[danakj, 2016-05-31 20:08:53]

On 2016/05/30 09:38:32, Thiemo Nagel (ooo until 5-23) wrote:
> On 2016/05/27 13:32:55, Nico (away until Tue May 31) wrote:
> > Why is this chromeos-specific? It doesn't make sense to me to have
> > platform-specific code in logging.h.
> 
> Chrome OS is different from all other platforms in that hitting NOTREACHED()
> logs an error message.  I believe that the extra code size was deemed
acceptable
> for Chrome OS but not for other platforms.

IIRC, I think it's because chromeos hits so many DCHECKs/NOTREACHED in practice
that the bots to not build with them enabled. So we have this rather unfortunate
situation where we just print out NOTREACHED and don't crash.