GCC 4.9 + Address Sanitizer + C-C TB: Compiling comm-central thunderbird using address sanitizer, -fsanitize=address using GCC 4.9

RESOLVED FIXED in Firefox 40

Status

()

Core
Memory Allocator
--
enhancement
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ISHIKAWA, Chiaki, Assigned: ISHIKAWA, Chiaki)

Tracking

unspecified
mozilla40
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8582822 [details] [diff] [review]
Making mozalloc.h friendly to GCC 4.9 and address sanitizer build

Hi,

Here is what I have done to use address sanitizer for C-C TB
using GCC 4.9.

It works now. 

I needed to patch a file and see if the patch is acceptable.
Comments welcome.

Motivation:

I said this before, and I say it again:
I want to have a rock solid mail client (tm).
I depend on TB for my office work where many mails with large attachments
flow.

TB still has a long way to go based on the judgment from the errors I
observe during local test, and from my own experience of real-world
errors in the past although it has improved quite a lot.
Well, I digress.

Particular issue: memory-related problem

In order to figure out some obscure memory error(s) of TB, I have
depended on valgrind/memcheck before. It helped me to uncover several
serious memory issues before.

Unfortunately, recently valgrind/memcheck has stopped working with
thunderbird.  Maybe it is due to underlying OS change, library
changes, etc.

While the current issue of valgrind/memcheck with mozilla software is
being looked into, as an alternative for memory testing, I could
compile C-C TB with GCC 4.9 and enable address sanitizer (ASAN).

This has already proved useful to track down a heap reference after
free error.

I don't know what I have done to compile TB with ASAN is optimal or
not, but here is what I did.

I would like to share this so that others can test TB or maybe
Firefox, too, in a similar manner using GCC and ASAN (CLANG and ASAN
is already working.)
There may be a developer or two who, for some reason or the other,
prefer to use GCC, or not have CLANG handy on their development machine..

Tips for improvement welcome.

(0) My environment

Debian GNU/Linux
$ uname -a
Linux ip030 3.16.0-4-amd64 #1 SMP Debian 3.16.7-ckt7-1 (2015-03-01) x86_64 GNU/Linux

GCC 4.9

$ gcc --version
gcc (Debian 4.9.2-10) 4.9.2
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

linker version

GNU gold (GNU Binutils 2.25.51.20141027) 1.11
Copyright (C) 2014 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or (at your option) a later version.
This program has absolutely no warranty.


(1) Prerequisite:

I added the following lines to
my mozconfig.
This is based on the suggestion for using address sanitizer with *clang*.

https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Firefox_and_Address_Sanitizer

Note that I am using GCC, but it is a start.
I would change setting as it becomes necessary to avoid issues when I
try building TB with ASAN using GCC 4.9.

I already smell something is fishy when I see LDFLAGS have
"-fsanitize=address" in the suggestion.
I doubt if this works with GCC.

In my case, I have already defined in my MOZCONFIG
the following:
># LINK speed OPTIMIZATION
>ac_add_options --enable-debug-symbols=-gsplit-dwarf

Also, to run valgrind, I already have defined
>ac_add_options --disable-jemalloc

--- addition with a slight modification ---
###
# address sanitizer for 64-bit linux
#

# Mandatory flags for ASan
export ASANFLAGS="-fsanitize=address -Dxmalloc=myxmalloc -fPIC"
export CFLAGS="$ASANFLAGS"
export CXXFLAGS="$ASANFLAGS"
export LDFLAGS="-fsanitize=address"

# Enable ASan specific code and build workarounds
ac_add_options --enable-address-sanitizer

# Mandatory options required for ASan builds (both on Linux and Mac)
export MOZ_DEBUG_SYMBOLS=1
#
# ac_add_options --enable-debug-symbols # already defined with -gsplit-dwarf  
#
ac_add_options --disable-install-strip
ac_add_options --disable-jemalloc
ac_add_options --disable-crashreporter
ac_add_options --disable-elf-hack

# Avoid dependency on libstdc++ 4.7
ac_add_options --enable-stdcxx-compat

Note that I commented out the suggested setting for
--enable-debug-symbols
because I already have a different line setting --enable-debug-symbols=-gsplit-dwarf

(2) First hurdle.

I ran |make -f client.mk| and saw link error as follows.

ERROR MESSAGE:
     ...
nsinstall_real
/REF-COMM-CENTRAL/comm-central/mozilla/nsprpub/config/now.c:35: error: undefined reference to '__asan_report_load8'
/REF-COMM-CENTRAL/comm-central/mozilla/nsprpub/config/now.c:39: error: undefined reference to '__asan_unregister_globals'
/REF-COMM-CENTRAL/comm-central/mozilla/nsprpub/config/now.c:39: error: undefined reference to '__asan_init_v3'
/REF-COMM-CENTRAL/comm-central/mozilla/nsprpub/config/now.c:39: error: undefined reference to '__asan_register_globals'
now.o(.debug_addr+0x40): error: undefined reference to '__asan_unregister_globals'
now.o(.debug_addr+0x48): error: undefined reference to '__asan_register_globals'
now.o(.debug_addr+0x50): error: undefined reference to '__asan_init_v3'
now.o(.debug_addr+0x70): error: undefined reference to '__asan_report_load8'
collect2: error: ld returned 1 exit status
Makefile:115: recipe for target 'now' failed
make[6]: *** [now] Error 1
/REF-COMM-CENTRAL/comm-central/mozilla/nsprpub/config/rules.mk:162: recipe for target 'export' failed
make[5]: *** [export] Error 2
Makefile:38: recipe for target 'export' failed
make[4]: *** [export] Error 2
/REF-COMM-CENTRAL/comm-central/mozilla/config/recurse.mk:82: recipe for target 'config/external/nspr/export' failed
make[3]: *** [config/external/nspr/export] Error 2
make[3]: *** Waiting for unfinished jobs....

So the linker somehow  does not link proper ASAN library. 
LDFLAGS setting in mozconfig seems ineffective for GCC.

(2.5) Check that if I have ASAN libraries installed.

To make sure I have a working ASAN environment,
I checked gcc-4.9.2 does work with -fsanitize=address.
[Well, I should probably have done this in the first place.]

I checked with a simple test program to see if ASAN works with gcc 4.9
and it does work.

E.g.:
gcc -v -fsanitize=address -o wrong-malloc wrong-malloc.c

---- Quote: wrong-malloc.c
#include <malloc.h>
#include <unistd.h>
#include <stdlib.h>

main()
{
  char *cp;
  cp = malloc(10);
  cp[0] = 1;
  cp[9] = 2; // tried cp[10] = 2; ASAN caught it, too. 
  free(cp);
  printf("%d\n", cp[0]); // ASAN caught this, too. 
  exit(EXIT_SUCCESS);
}
----

I get proper error diagnostics.
So I think I have proper ASAN libraries installed.
Setting LDFLAGS in mozconfig seems to be useless for GCC.

While posting this bugzilla entry, I noticed that there is a similar
comment that LDFLAG is not useful in last November (for clang, too?)
https://bugzilla.mozilla.org/show_bug.cgi?id=1084157#c8

(3) First hurdle solved.

I figured out that if I simply add "-fsanitize=address" to CC and CXX
macros that I set just before I run

   make -f client.mk

I can solve this missing symbol problem for now.c.
In my case, luckily, I am setting CC and CXX with a few macros
already for my local debugging purposes.

So here is the gist of setting in my build shell script now.

--- begin quote

    ...

SPLITDWARF="-Wl,--gdb-index"
ASAN=-fsanitize=address
MEMORYMODEL=

        ...

export CC="/usr/bin/gcc-4.9  $MEMORYMODEL $ASAN -fno-builtin-strlen $SPLITDWARF -Dfdatasync=fdatasync -DDEBUG=1 -DDEBUG_4GB_CHECK -DUSEHELGRIND=1"
export CXX="/usr/bin/g++-4.9 $MEMORYMODEL $ASAN  -fno-builtin-strlen $SPLITDWARF -Dfdatasync=fdatasync -DDEBUG=1 -DDEBUG_4GB_CHECK -DUSEHELGRIND=1"

       ...

time make -f client.mk
--- end quote

Actually with the following lines in my MOZCONFIG, the above setting
seems to put "-fsanitize=address" twice, at least, in GCC option during compilation of
many files. Maybe new.c is compiled in a slightly different manner.
But I can't help it here.


[BTW, in the early 4.8.x days, I tried adding -fsanitize=address to
these CC and CXX macros, but it did not seem to work then. Maybe I
forgot to add ASAN-related libraries, or Debian GNU/Linux may not have
packaged ASAN libraries and provided as part of GCC installation
properly.  
Of course, I did not have additional setting in mozconfig back then.
So ASAN was set to "" (empty string) until yesterday.]

(4) Second Hurdle.

I ran |make -f client.mk| with modified CC, CCX macros, etc.

Then further down during build (now.c is taken care of), I saw the
failed compilation of nsComponentManager.cpp

I think the mozmalloc.h used for this special compilation is not
quite friendly to GCC 4.9.

ERROR MESSAGE:
   ...
In file included from /REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/../base/nscore.h:20:0,
                 from /REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/nsComponentManager.cpp:20:
../../dist/include/mozilla/mozalloc.h: In static member function ‘static void nsComponentManagerImpl::InitializeStaticModules()’:
../../dist/include/mozilla/mozalloc.h:194:7: error: inlining failed in call to always_inline ‘void* operator new(std::size_t)’: function attribute mismatch
 void* operator new(size_t size) MOZALLOC_THROW_BAD_ALLOC
       ^
/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/nsComponentManager.cpp:337:56: error: called from here
   sStaticModules = new nsTArray<const mozilla::Module*>;
                                                        ^

In the directory  /REF-OBJ-DIR/objdir-tb3/xpcom/components
The following command failed to execute properly:
/usr/bin/ccache /usr/bin/g++-4.9 -fsanitize=address -fno-builtin-strlen -Wl,--gdb-index -Dfdatasync=fdatasync -DDEBUG=1 -DDEBUG_4GB_CHECK -DUSEHELGRIND=1 -o nsComponentManager.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /REF-COMM-CENTRAL/comm-central/mozilla/config/gcc_hidden.h -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DMOZ_GLUE_IN_PROGRAM -DAB_CD=en-US -DNO_NSPR_10_SUPPORT -I/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components -I. -I.. -I/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/../base -I/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/../build -I/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/../ds -I/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/../reflect/xptinfo -I/REF-COMM-CENTRAL/comm-central/mozilla/chrome -I/REF-COMM-CENTRAL/comm-central/mozilla/modules/libjar -I../../dist/include -I/REF-OBJ-DIR/objdir-tb3/dist/include/nspr -I/REF-OBJ-DIR/objdir-tb3/dist/include/nss -fPIC -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/nsComponentManager.o.pp -Wall -Wempty-body -Woverloaded-virtual -Wsign-compare -Wwrite-strings -Wno-invalid-offsetof -Wcast-align -fsanitize=address -Dxmalloc=myxmalloc -fPIC -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe -DDEBUG -DTRACING -gsplit-dwarf -g -O -fsanitize=address -fno-omit-frame-pointer -freorder-blocks -fno-omit-frame-pointer -I/REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/compat -pthread -I/usr/include/gtk-2.0 -I/usr/include/gtk-unix-print-2.0 -I/usr/include/gtk-2.0 -I/usr/include/atk-1.0 -I/usr/include/pango-1.0 -I/usr/include/harfbuzz -I/usr/include/gtk-2.0 -I/usr/lib/x86_64-linux-gnu/gtk-2.0/include -I/usr/include/gio-unix-2.0/ -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libpng12 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include /REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/nsComponentManager.cpp
/REF-COMM-CENTRAL/comm-central/mozilla/config/rules.mk:930: recipe for target 'nsComponentManager.o' failed
make[4]: *** [nsComponentManager.o] Error 1
/REF-COMM-CENTRAL/comm-central/mozilla/config/recurse.mk:74: recipe for target 'xpcom/components/target' failed
make[3]: *** [xpcom/components/target] Error 2
make[3]: *** Waiting for unfinished jobs....


(3) Second Hurdle solved with a kludge.

I looked at mozmalloc.h and modified it.
I bit the bullet and decided NOT to inline new() in a manner not
friendly to GCC 4.9

For whatever the reason, I think |new| is declared with an
incompatible attribute for inlining  when -Dxmalloc=myxmalloc is done.

So I changed the source for now as follows (not quite correct as you
see in the next step):

diff --git a/memory/mozalloc/mozalloc.h b/memory/mozalloc/mozalloc.h
--- a/memory/mozalloc/mozalloc.h
+++ b/memory/mozalloc/mozalloc.h
@@ -185,17 +185,20 @@ MFBT_API void* moz_valloc(size_t size)
 #define MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS
 #else
 #define MOZALLOC_THROW_IF_HAS_EXCEPTIONS throw()
 #define MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS throw(std::bad_alloc)
 #endif
 
 #define MOZALLOC_THROW_BAD_ALLOC MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS
 
-MOZALLOC_EXPORT_NEW MOZALLOC_INLINE
+MOZALLOC_EXPORT_NEW 
+#if !defined(xmalloc)
+MOZALLOC_INLINE
+#endif
 void* operator new(size_t size) MOZALLOC_THROW_BAD_ALLOC
 {
     return moz_xmalloc(size);
 }
 
 MOZALLOC_EXPORT_NEW MOZALLOC_INLINE
 void* operator new(size_t size, const std::nothrow_t&) MOZALLOC_THROW_IF_HAS_EXCEPTIONS
 {


(4) Third Hurdle  

The previous fix for |new| was not quite enough.
Further down the road during build, I see the following error.

The problem is the multiple definition of function |new|.
It looks I *do* NEED to inline |new| or I get this multiple definition.

ERROR MESSAGE:

: /REF-OBJ-DIR/objdir-tb3/testing/mochitest/ssltunnel/../../../memory/mozalloc/Unified_cpp_memory_mozalloc0.o: multiple definition of 'operator new(unsigned long)'
/usr/bin/ld.new: /REF-OBJ-DIR/objdir-tb3/testing/mochitest/ssltunnel/ssltunnel.o: previous definition here

...
Executing: /usr/bin/ccache /usr/bin/g++-4.9 -fsanitize=address -fno-builtin-strlen -Wl,--gdb-index -Dfdatasync=fdatasync -DDEBUG=1 -DDEBUG_4GB_CHECK -DUSEHELGRIND=1 -o ssltunnel -Wall -Wempty-body -Woverloaded-virtual -Wsign-compare -Wwrite-strings -Wno-invalid-offsetof -Wcast-align -fsanitize=address -Dxmalloc=myxmalloc -fPIC -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe -DDEBUG -DTRACING -gsplit-dwarf -g -O -fsanitize=address -fno-omit-frame-pointer -freorder-blocks -fno-omit-frame-pointer /REF-OBJ-DIR/objdir-tb3/testing/mochitest/ssltunnel/tmpwj8ogs.list -lpthread -fsanitize=address -Wl,-z,noexecstack -Wl,-z,text -Wl,--build-id -rdynamic -Wl,-rpath-link,/REF-OBJ-DIR/objdir-tb3/dist/bin -Wl,-rpath-link,/usr/local/lib ../../../nsprpub/lib/ds/libplds4.so ../../../nsprpub/lib/libc/src/libplc4.so ../../../nsprpub/pr/src/libnspr4.so ../../../security/nss/lib/nss/libnss3.so ../../../security/nss/lib/smime/libsmime3.so ../../../security/nss/lib/ssl/libssl3.so ../../../security/nss/lib/util/libnssutil3.so ../../../config/external/sqlite/libmozsqlite3.so -ldl
/REF-OBJ-DIR/objdir-tb3/testing/mochitest/ssltunnel/tmpwj8ogs.list:
    INPUT("ssltunnel.o")
    INPUT("../../../build/unix/stdc++compat/stdc++compat.o")
    INPUT("../../../mozglue/build/AsanOptions.o")
    INPUT("../../../mozglue/build/SSE.o")
    INPUT("../../../mozglue/build/dummy.o")
    INPUT("../../../memory/mozalloc/Unified_cpp_memory_mozalloc0.o")
    INPUT("../../../mfbt/Compression.o")
    INPUT("../../../mfbt/Decimal.o")
    INPUT("../../../mfbt/Unified_cpp_mfbt0.o")
    INPUT("../../../memory/fallible/fallible.o")

/usr/bin/ld.new: error: /REF-OBJ-DIR/objdir-tb3/testing/mochitest/ssltunnel/../../../memory/mozalloc/Unified_cpp_memory_mozalloc0.o: multiple definition of 'operator new(unsigned long)'
/usr/bin/ld.new: /REF-OBJ-DIR/objdir-tb3/testing/mochitest/ssltunnel/ssltunnel.o: previous definition here
collect2: error: ld returned 1 exit status
/REF-COMM-CENTRAL/comm-central/mozilla/config/rules.mk:658: recipe for target 'ssltunnel' failed
make[4]: *** [ssltunnel] Error 1
/REF-COMM-CENTRAL/comm-central/mozilla/config/recurse.mk:74: recipe for target 'testing/mochitest/ssltunnel/target' failed
make[3]: *** [testing/mochitest/ssltunnel/target] Error 2
make[3]: *** Waiting for unfinished jobs....

[20/20 Hindsight: It looks this happened in testing directory. Maybe I
could have ignored this issue...]

(5) Back to documentation.

I read the following URL very carefully.

https://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html

This explains |inline| et al.

It looks to me declaring |new| in mozmalloc.h
as

extern
inline
__attribute((gnu_inline))

may be the way to go.

Pretty complex and subtle, eh?
(It is to inline as much as possible, but do not
create a function body even if the address of the function is taken,
etc. according to the doc.)

By sheer coincidence, while I am building TB with this modification of
mine, I read this e-mail message in gdb mailing list which says:
( in the follow up to a link failure question:
http://comments.gmane.org/gmane.comp.gdb.devel/35494 )

--- begin quote ---
what gcc version are you using ?  there might be some misbehavior with the usage 
of extern inline.  i think the code base expects gnu_inline semantics but just 
uses "extern inline" directly.  maybe try including -fgnu89-inline in your 
CFLAGS and see if that helps ?
--- end quote

I think we need |gnu_inline| semantics here considering the
multiple link against ssl library.

This is the new modification of mozmalloc.h.


diff --git a/memory/mozalloc/mozalloc.h b/memory/mozalloc/mozalloc.h
--- a/memory/mozalloc/mozalloc.h
+++ b/memory/mozalloc/mozalloc.h
@@ -185,21 +185,37 @@ MFBT_API void* moz_valloc(size_t size)
 #define MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS
 #else
 #define MOZALLOC_THROW_IF_HAS_EXCEPTIONS throw()
 #define MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS throw(std::bad_alloc)
 #endif
 
 #define MOZALLOC_THROW_BAD_ALLOC MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS
 
-MOZALLOC_EXPORT_NEW MOZALLOC_INLINE
-void* operator new(size_t size) MOZALLOC_THROW_BAD_ALLOC
+#if defined(__GNUC__)
+MOZALLOC_EXPORT_NEW 
+extern
+inline
+__attribute__((gnu_inline))
+void* operator new(size_t size)
+    MOZALLOC_THROW_BAD_ALLOC
 {
     return moz_xmalloc(size);
 }
+#else
+MOZALLOC_EXPORT_NEW 
+MOZALLOC_INLINE
+void* operator new(size_t size)
+    MOZALLOC_THROW_BAD_ALLOC
+{
+    return moz_xmalloc(size);
+}
+
+#endif
+
 
 MOZALLOC_EXPORT_NEW MOZALLOC_INLINE
 void* operator new(size_t size, const std::nothrow_t&) MOZALLOC_THROW_IF_HAS_EXCEPTIONS
 {
     return moz_malloc(size);
 }
 
 MOZALLOC_EXPORT_NEW MOZALLOC_INLINE

(6) That's it.

C-C TB was compiled with ASAN this way, and ran successfully (!)

Already, I could find out an issue in

> #0 0x7f25b74d5c8b in nsMsgFilterAfterTheFact::OnEndExecution() (/REF-OBJ-DIR/objdir-tb3/dist/bin/libxul.so+0x21a0c8b)

where freed memory (member field in a released class object)
is referenced which rkent had found by code inspection !


TIA
(Assignee)

Comment 1

3 years ago
Comment on attachment 8582822 [details] [diff] [review]
Making mozalloc.h friendly to GCC 4.9 and address sanitizer build

Fixed a typo in the name of the file being patched.
Attachment #8582822 - Attachment description: Making mozmalloc.h friendly to GCC 4.9 and address sanitizer build → Making mozalloc.h friendly to GCC 4.9 and address sanitizer build
(Assignee)

Comment 2

3 years ago
Comment on attachment 8582822 [details] [diff] [review]
Making mozalloc.h friendly to GCC 4.9 and address sanitizer build

I am asking ms2ger who touched the said portion of the code previously.
Thank you in advance.
Attachment #8582822 - Flags: review?(Ms2ger)
Comment on attachment 8582822 [details] [diff] [review]
Making mozalloc.h friendly to GCC 4.9 and address sanitizer build

Review of attachment 8582822 [details] [diff] [review]:
-----------------------------------------------------------------

You've got trailing whitespace.
Attachment #8582822 - Flags: review?(Ms2ger) → review?(mh+mozilla)

Comment 4

3 years ago
Comment on attachment 8582822 [details] [diff] [review]
Making mozalloc.h friendly to GCC 4.9 and address sanitizer build

Why not change MOZALLOC_INLINE or MOZ_ALWAYS_INLINE_EVEN_DEBUG to use __attribute__((gnu_inline, always_inline))?

Comment 5

3 years ago
This patch is probably wrong since clang also defines __GNUC__.
(Assignee)

Comment 6

3 years ago
Created attachment 8584054 [details] [diff] [review]
Making mozalloc.h friendly to GCC 4.9 and address sanitizer build  (Take 2)

(In reply to :Ms2ger from comment #3)
> Comment on attachment 8582822 [details] [diff] [review]
> Making mozalloc.h friendly to GCC 4.9 and address sanitizer build
> 
> Review of attachment 8582822 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You've got trailing whitespace.

Thank you for noticing this.
I took care of that and changed the expression on #if based on comment 5.

(In reply to neil@parkwaycc.co.uk from comment #4)
> Comment on attachment 8582822 [details] [diff] [review]
> Making mozalloc.h friendly to GCC 4.9 and address sanitizer build
> 
> Why not change MOZALLOC_INLINE or MOZ_ALWAYS_INLINE_EVEN_DEBUG to use
> __attribute__((gnu_inline, always_inline))?

Thank you for the suggestion.
Why not indeed. I did not know exactly what these macros are for.

Now, please recall that only one form of the declaration of new(), which is being modified,
caused duplicate definition error during link for a test in SSH-related library.

Unfortunately, I found out that, if I use the suggested attributes, I got tons of warnings for
|gnu_inline| being ignored (I think I need |extern| to go with it for GCC, which is a peculiar thing (and ugly) but the referred GCC web page is clear about this.),

E.g., warnings:
...
../../../dist/include/mozilla/mozalloc.h:210:62: warning: ‘gnu_inline’ attribute ignored [-Wattributes]
 void* operator new(size_t size, const std::nothrow_t&) MOZALLOC_THROW_IF_HAS_EXCEPTIONS
                                                              ^
../../../dist/include/mozilla/mozalloc.h:216:55: warning: ‘gnu_inline’ attribute ignored [-Wattributes]
 void* operator new[](size_t size) MOZALLOC_THROW_BAD_ALLOC
                                                       ^
../../../dist/include/mozilla/mozalloc.h:222:64: warning: ‘gnu_inline’ attribute ignored [-Wattributes]
 void* operator new[](size_t size, const std::nothrow_t&) MOZALLOC_THROW_IF_HAS_EXCEPTIONS
...                                                                ^

 and eventually get a compilation error because of the following:

E.g.: warnings continued and then Error

...
../../dist/include/mozilla/mozalloc.h:203:7: warning: always_inline function might not be inlinable [-Wattributes]
 void* operator new(size_t size)
       ^
../../dist/include/mozilla/mozalloc.h: In destructor ‘mozilla::dom::PresentationDeviceManager::~PresentationDeviceManager()’:
../../dist/include/mozilla/mozalloc.h:228:6: error: inlining failed in call to always_inline ‘void operator delete(void*) noexcept’: function body can be overwritten at link time
 void operator delete(void* ptr) MOZALLOC_THROW_IF_HAS_EXCEPTIONS
      ^
/REF-COMM-CENTRAL/comm-central/mozilla/dom/presentation/PresentationDeviceManager.cpp:34:1: error: called from here
 }

...


So always_inline needs to be out because of the error.

Because of the too numerous warnings, I had to add |extern| and 
I did not want to
disturb the declarations of functions other than the particular new().

This is why I ended up with this patch.


(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #5)
> This patch is probably wrong since clang also defines __GNUC__.

Thank you for this information. I did not know this. So defined(__clang__) needs to 
be used to differentiate pure GCC and clang usage.
I have installed clang and hopefully can check the operation of ASAN using clang in the next several weeks.
Until then, gcc fills the gap for now.

Attached is the modified patch.

TIA
Assignee: nobody → ishikawa
Attachment #8582822 - Attachment is obsolete: true
Attachment #8582822 - Flags: review?(mh+mozilla)
Attachment #8584054 - Flags: review?(mh+mozilla)
Comment on attachment 8584054 [details] [diff] [review]
Making mozalloc.h friendly to GCC 4.9 and address sanitizer build  (Take 2)

Review of attachment 8584054 [details] [diff] [review]:
-----------------------------------------------------------------

::: memory/mozalloc/mozalloc.h
@@ +164,5 @@
>   * exported. */
>  #if defined(XP_MACOSX)
>  #  define MOZALLOC_EXPORT_NEW MFBT_API
>  #else
> +#  define MOZALLOC_EXPORT_NEW 

Why add a trailing whitespace here?

@@ +189,5 @@
>  #endif
>  
>  #define MOZALLOC_THROW_BAD_ALLOC MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS
>  
> +#if ! (defined(__GNUC__) && !defined(__clang__))

This effectively makes all gcc builds not use MOZALLOC_INLINE. I don't think we want that. Please make that only happen when -fsanitize=address is given (which sets __SANITIZE_ADDRESS__ with gcc)

@@ +198,5 @@
>  {
>      return moz_xmalloc(size);
>  }
> +#else
> +/* GCC + ASAN change.: Bug 1147248 */

Could you summarize why this is needed here? Because comment 0 is not really useful for that, as it contains too many unrelated things to grasp what you're trying to fix in concrete terms.

@@ +202,5 @@
> +/* GCC + ASAN change.: Bug 1147248 */
> +MOZALLOC_EXPORT_NEW
> +extern
> +inline
> +__attribute__((gnu_inline))

Since you're only really changing MOZALLOC_INLINE, please only change MOZALLOC_INLINE.

(like:
MOZALLOC_EXPORT_NEW
#if ...
MOZALLOC_INLINE
#else
extern inline __attribute__((gnu_inline))
#endif
Attachment #8584054 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 8

3 years ago
(In reply to Mike Hommey [:glandium] from comment #7)
> Comment on attachment 8584054 [details] [diff] [review]
> Making mozalloc.h friendly to GCC 4.9 and address sanitizer build  (Take 2)
> 
> Review of attachment 8584054 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: memory/mozalloc/mozalloc.h
> @@ +164,5 @@
> >   * exported. */
> >  #if defined(XP_MACOSX)
> >  #  define MOZALLOC_EXPORT_NEW MFBT_API
> >  #else
> > +#  define MOZALLOC_EXPORT_NEW 
> 
> Why add a trailing whitespace here?

I am afraid my fingers slipped.

> 
> @@ +189,5 @@
> >  #endif
> >  
> >  #define MOZALLOC_THROW_BAD_ALLOC MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS
> >  
> > +#if ! (defined(__GNUC__) && !defined(__clang__))
> 
> This effectively makes all gcc builds not use MOZALLOC_INLINE. I don't think
> we want that. Please make that only happen when -fsanitize=address is given
> (which sets __SANITIZE_ADDRESS__ with gcc)

OK I will use this macro,  __SANITIZE_ADDRESS__ for a new patch.

> @@ +198,5 @@
> >  {
> >      return moz_xmalloc(size);
> >  }
> > +#else
> > +/* GCC + ASAN change.: Bug 1147248 */
> 
> Could you summarize why this is needed here? Because comment 0 is not really
> useful for that, as it contains too many unrelated things to grasp what
> you're trying to fix in concrete terms.

Sorry, comment 0 contains the whole procedure of using GCC with address sanitizer.

The gist of the reason why this change is necessary is quoted again below: longish, sorry.
In a nutshell, if my understanding is correct, a version of new with a particular signature is declared
in an unfriendly manner to GCC, and it requires gnu_inline semantics.

--- begin quote ---
(4) Second Hurdle.

I ran |make -f client.mk| with modified CC, CCX macros, etc.

Then further down during build (now.c is taken care of), I saw the
failed compilation of nsComponentManager.cpp

I think the mozmalloc.h used for this special compilation is not
quite friendly to GCC 4.9.

ERROR MESSAGE:
   ...
In file included from /REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/../base/nscore.h:20:0,
                 from /REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/nsComponentManager.cpp:20:
../../dist/include/mozilla/mozalloc.h: In static member function ‘static void nsComponentManagerImpl::InitializeStaticModules()’:
../../dist/include/mozilla/mozalloc.h:194:7: error: inlining failed in call to always_inline ‘void* operator new(std::size_t)’: function attribute mismatch
 void* operator new(size_t size) MOZALLOC_THROW_BAD_ALLOC
       ^
/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/nsComponentManager.cpp:337:56: error: called from here
   sStaticModules = new nsTArray<const mozilla::Module*>;
                                                        ^

In the directory  /REF-OBJ-DIR/objdir-tb3/xpcom/components
The following command failed to execute properly:
/usr/bin/ccache /usr/bin/g++-4.9 -fsanitize=address -fno-builtin-strlen -Wl,--gdb-index -Dfdatasync=fdatasync -DDEBUG=1 -DDEBUG_4GB_CHECK -DUSEHELGRIND=1 -o nsComponentManager.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /REF-COMM-CENTRAL/comm-central/mozilla/config/gcc_hidden.h -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DMOZ_GLUE_IN_PROGRAM -DAB_CD=en-US -DNO_NSPR_10_SUPPORT -I/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components -I. -I.. -I/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/../base -I/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/../build -I/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/../ds -I/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/../reflect/xptinfo -I/REF-COMM-CENTRAL/comm-central/mozilla/chrome -I/REF-COMM-CENTRAL/comm-central/mozilla/modules/libjar -I../../dist/include -I/REF-OBJ-DIR/objdir-tb3/dist/include/nspr -I/REF-OBJ-DIR/objdir-tb3/dist/include/nss -fPIC -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/nsComponentManager.o.pp -Wall -Wempty-body -Woverloaded-virtual -Wsign-compare -Wwrite-strings -Wno-invalid-offsetof -Wcast-align -fsanitize=address -Dxmalloc=myxmalloc -fPIC -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe -DDEBUG -DTRACING -gsplit-dwarf -g -O -fsanitize=address -fno-omit-frame-pointer -freorder-blocks -fno-omit-frame-pointer -I/REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/compat -pthread -I/usr/include/gtk-2.0 -I/usr/include/gtk-unix-print-2.0 -I/usr/include/gtk-2.0 -I/usr/include/atk-1.0 -I/usr/include/pango-1.0 -I/usr/include/harfbuzz -I/usr/include/gtk-2.0 -I/usr/lib/x86_64-linux-gnu/gtk-2.0/include -I/usr/include/gio-unix-2.0/ -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libpng12 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include /REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/nsComponentManager.cpp
/REF-COMM-CENTRAL/comm-central/mozilla/config/rules.mk:930: recipe for target 'nsComponentManager.o' failed
make[4]: *** [nsComponentManager.o] Error 1
/REF-COMM-CENTRAL/comm-central/mozilla/config/recurse.mk:74: recipe for target 'xpcom/components/target' failed
make[3]: *** [xpcom/components/target] Error 2
make[3]: *** Waiting for unfinished jobs....


(3) Second Hurdle solved with a kludge.

I looked at mozmalloc.h and modified it.
I bit the bullet and decided NOT to inline new() in a manner not
friendly to GCC 4.9

For whatever the reason, I think |new| is declared with an
incompatible attribute for inlining  when -Dxmalloc=myxmalloc is done.

So I changed the source for now as follows (not quite correct as you
see in the next step):

diff --git a/memory/mozalloc/mozalloc.h b/memory/mozalloc/mozalloc.h
--- a/memory/mozalloc/mozalloc.h
+++ b/memory/mozalloc/mozalloc.h
@@ -185,17 +185,20 @@ MFBT_API void* moz_valloc(size_t size)
 #define MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS
 #else
 #define MOZALLOC_THROW_IF_HAS_EXCEPTIONS throw()
 #define MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS throw(std::bad_alloc)
 #endif
 
 #define MOZALLOC_THROW_BAD_ALLOC MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS
 
-MOZALLOC_EXPORT_NEW MOZALLOC_INLINE
+MOZALLOC_EXPORT_NEW 
+#if !defined(xmalloc)
+MOZALLOC_INLINE
+#endif
 void* operator new(size_t size) MOZALLOC_THROW_BAD_ALLOC
 {
     return moz_xmalloc(size);
 }
 
 MOZALLOC_EXPORT_NEW MOZALLOC_INLINE
 void* operator new(size_t size, const std::nothrow_t&) MOZALLOC_THROW_IF_HAS_EXCEPTIONS
 {


(4) Third Hurdle  

The previous fix for |new| was not quite enough.
Further down the road during build, I see the following error.

The problem is the multiple definition of function |new|.
It looks I *do* NEED to inline |new| or I get this multiple definition.

ERROR MESSAGE:

: /REF-OBJ-DIR/objdir-tb3/testing/mochitest/ssltunnel/../../../memory/mozalloc/Unified_cpp_memory_mozalloc0.o: multiple definition of 'operator new(unsigned long)'
/usr/bin/ld.new: /REF-OBJ-DIR/objdir-tb3/testing/mochitest/ssltunnel/ssltunnel.o: previous definition here

...
Executing: /usr/bin/ccache /usr/bin/g++-4.9 -fsanitize=address -fno-builtin-strlen -Wl,--gdb-index -Dfdatasync=fdatasync -DDEBUG=1 -DDEBUG_4GB_CHECK -DUSEHELGRIND=1 -o ssltunnel -Wall -Wempty-body -Woverloaded-virtual -Wsign-compare -Wwrite-strings -Wno-invalid-offsetof -Wcast-align -fsanitize=address -Dxmalloc=myxmalloc -fPIC -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe -DDEBUG -DTRACING -gsplit-dwarf -g -O -fsanitize=address -fno-omit-frame-pointer -freorder-blocks -fno-omit-frame-pointer /REF-OBJ-DIR/objdir-tb3/testing/mochitest/ssltunnel/tmpwj8ogs.list -lpthread -fsanitize=address -Wl,-z,noexecstack -Wl,-z,text -Wl,--build-id -rdynamic -Wl,-rpath-link,/REF-OBJ-DIR/objdir-tb3/dist/bin -Wl,-rpath-link,/usr/local/lib ../../../nsprpub/lib/ds/libplds4.so ../../../nsprpub/lib/libc/src/libplc4.so ../../../nsprpub/pr/src/libnspr4.so ../../../security/nss/lib/nss/libnss3.so ../../../security/nss/lib/smime/libsmime3.so ../../../security/nss/lib/ssl/libssl3.so ../../../security/nss/lib/util/libnssutil3.so ../../../config/external/sqlite/libmozsqlite3.so -ldl
/REF-OBJ-DIR/objdir-tb3/testing/mochitest/ssltunnel/tmpwj8ogs.list:
    INPUT("ssltunnel.o")
    INPUT("../../../build/unix/stdc++compat/stdc++compat.o")
    INPUT("../../../mozglue/build/AsanOptions.o")
    INPUT("../../../mozglue/build/SSE.o")
    INPUT("../../../mozglue/build/dummy.o")
    INPUT("../../../memory/mozalloc/Unified_cpp_memory_mozalloc0.o")
    INPUT("../../../mfbt/Compression.o")
    INPUT("../../../mfbt/Decimal.o")
    INPUT("../../../mfbt/Unified_cpp_mfbt0.o")
    INPUT("../../../memory/fallible/fallible.o")

/usr/bin/ld.new: error: /REF-OBJ-DIR/objdir-tb3/testing/mochitest/ssltunnel/../../../memory/mozalloc/Unified_cpp_memory_mozalloc0.o: multiple definition of 'operator new(unsigned long)'
/usr/bin/ld.new: /REF-OBJ-DIR/objdir-tb3/testing/mochitest/ssltunnel/ssltunnel.o: previous definition here
collect2: error: ld returned 1 exit status
/REF-COMM-CENTRAL/comm-central/mozilla/config/rules.mk:658: recipe for target 'ssltunnel' failed
make[4]: *** [ssltunnel] Error 1
/REF-COMM-CENTRAL/comm-central/mozilla/config/recurse.mk:74: recipe for target 'testing/mochitest/ssltunnel/target' failed
make[3]: *** [testing/mochitest/ssltunnel/target] Error 2
make[3]: *** Waiting for unfinished jobs....

[20/20 Hindsight: It looks this happened in testing directory. Maybe I
could have ignored this issue...]

(5) Back to documentation.

I read the following URL very carefully.

https://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html

This explains |inline| et al.

It looks to me declaring |new| in mozmalloc.h
as

extern
inline
__attribute((gnu_inline))

may be the way to go.

Pretty complex and subtle, eh?
(It is to inline as much as possible, but do not
create a function body even if the address of the function is taken,
etc. according to the doc.)

By sheer coincidence, while I am building TB with this modification of
mine, I read this e-mail message in gdb mailing list which says:
( in the follow up to a link failure question:
http://comments.gmane.org/gmane.comp.gdb.devel/35494 )

--- begin quote ---
what gcc version are you using ?  there might be some misbehavior with the usage 
of extern inline.  i think the code base expects gnu_inline semantics but just 
uses "extern inline" directly.  maybe try including -fgnu89-inline in your 
CFLAGS and see if that helps ?
--- end quote

I think we need |gnu_inline| semantics here considering the
multiple link against ssl library.

This is the new modification of mozmalloc.h.


diff --git a/memory/mozalloc/mozalloc.h b/memory/mozalloc/mozalloc.h
--- a/memory/mozalloc/mozalloc.h
+++ b/memory/mozalloc/mozalloc.h
@@ -185,21 +185,37 @@ MFBT_API void* moz_valloc(size_t size)
 #define MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS
 #else
 #define MOZALLOC_THROW_IF_HAS_EXCEPTIONS throw()
 #define MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS throw(std::bad_alloc)
 #endif
 
 #define MOZALLOC_THROW_BAD_ALLOC MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS
 
-MOZALLOC_EXPORT_NEW MOZALLOC_INLINE
-void* operator new(size_t size) MOZALLOC_THROW_BAD_ALLOC
+#if defined(__GNUC__)
+MOZALLOC_EXPORT_NEW 
+extern
+inline
+__attribute__((gnu_inline))
+void* operator new(size_t size)
+    MOZALLOC_THROW_BAD_ALLOC
 {
     return moz_xmalloc(size);
 }
+#else
+MOZALLOC_EXPORT_NEW 
+MOZALLOC_INLINE
+void* operator new(size_t size)
+    MOZALLOC_THROW_BAD_ALLOC
+{
+    return moz_xmalloc(size);
+}
+
+#endif
+
 
 MOZALLOC_EXPORT_NEW MOZALLOC_INLINE
 void* operator new(size_t size, const std::nothrow_t&) MOZALLOC_THROW_IF_HAS_EXCEPTIONS
 {
     return moz_malloc(size);
 }
 
 MOZALLOC_EXPORT_NEW MOZALLOC_INLINE
--- end quote ---


> @@ +202,5 @@
> > +/* GCC + ASAN change.: Bug 1147248 */
> > +MOZALLOC_EXPORT_NEW
> > +extern
> > +inline
> > +__attribute__((gnu_inline))
> 
> Since you're only really changing MOZALLOC_INLINE, please only change
> MOZALLOC_INLINE.
> 
> (like:
> MOZALLOC_EXPORT_NEW
> #if ...
> MOZALLOC_INLINE
> #else
> extern inline __attribute__((gnu_inline))
> #endif

Will do to create a new patch later today.

TIA
(Assignee)

Comment 9

3 years ago
Created attachment 8586805 [details] [diff] [review]
Attachments Making mozalloc.h friendly to GCC 4.9 and address sanitizer build (Take 3)

Here is a new patch that contains the suggested changes.
(Compiled locally to make sure ASAN with GCC and can catch memory errors.)

TIA
Attachment #8584054 - Attachment is obsolete: true
Attachment #8586805 - Flags: review?(mh+mozilla)
Comment on attachment 8586805 [details] [diff] [review]
Attachments Making mozalloc.h friendly to GCC 4.9 and address sanitizer build (Take 3)

Review of attachment 8586805 [details] [diff] [review]:
-----------------------------------------------------------------

::: memory/mozalloc/mozalloc.h
@@ +194,5 @@
> +MOZALLOC_EXPORT_NEW
> +#if ! (defined(__GNUC__) && !defined(__clang__) && defined(__SANITIZE_ADDRESS__))
> +MOZALLOC_INLINE
> +#else
> +extern inline __attribute__((gnu_inline))

As far as my testing goes, "extern" is not needed. Please put "inline" after the __attribute__, just to have the same order as MOZ_ALWAYS_INLINE_EVEN_DEBUG (which is what MOZALLOC_INLINE expands to).

Please also add a comment that this is because gcc's asan somehow doesn't like always_inline on this function.
Attachment #8586805 - Flags: review?(mh+mozilla) → feedback+
(Assignee)

Comment 11

3 years ago
Created attachment 8589748 [details] [diff] [review]
Making mozalloc.h friendly to GCC 4.9 and address sanitizer build (Take 4)

(In reply to Mike Hommey [:glandium] from comment #10)
> Comment on attachment 8586805 [details] [diff] [review]
> Attachments Making mozalloc.h friendly to GCC 4.9 and address sanitizer
> build (Take 3)
> 
> Review of attachment 8586805 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: memory/mozalloc/mozalloc.h
> @@ +194,5 @@
> > +MOZALLOC_EXPORT_NEW
> > +#if ! (defined(__GNUC__) && !defined(__clang__) && defined(__SANITIZE_ADDRESS__))
> > +MOZALLOC_INLINE
> > +#else
> > +extern inline __attribute__((gnu_inline))
> 
> As far as my testing goes, "extern" is not needed. Please put "inline" after
> the __attribute__, just to have the same order as
> MOZ_ALWAYS_INLINE_EVEN_DEBUG (which is what MOZALLOC_INLINE expands to).
> 
> Please also add a comment that this is because gcc's asan somehow doesn't
> like always_inline on this function.

Incoporated the above requests.

Yes, I also found "extern" is not necessary despite what GCC manual page says.
Oh well.

I reversed the #if expr so that it is more intuitive.

TIA
Attachment #8586805 - Attachment is obsolete: true
Attachment #8589748 - Flags: review?(mh+mozilla)
Comment on attachment 8589748 [details] [diff] [review]
Making mozalloc.h friendly to GCC 4.9 and address sanitizer build (Take 4)

Review of attachment 8589748 [details] [diff] [review]:
-----------------------------------------------------------------

::: memory/mozalloc/mozalloc.h
@@ +176,5 @@
>  #define MOZALLOC_THROW_BAD_ALLOC MOZALLOC_THROW_BAD_ALLOC_IF_HAS_EXCEPTIONS
>  
> +
> +MOZALLOC_EXPORT_NEW
> +#if (defined(__GNUC__) && !defined(__clang__) && defined(__SANITIZE_ADDRESS__))

You could remove the enclosing parentheses: #if defined(__GNUC__) && ...

@@ +178,5 @@
> +
> +MOZALLOC_EXPORT_NEW
> +#if (defined(__GNUC__) && !defined(__clang__) && defined(__SANITIZE_ADDRESS__))
> +/* gcc's asan somehow doesn't like always_inline on this function. */
> +__attribute__((gnu_inline)) inline 

Please remove the trailing whitespace.
Attachment #8589748 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 13

3 years ago
Created attachment 8591111 [details] [diff] [review]
Making mozalloc.h friendly to GCC 4.9 and address sanitizer build (Take 5) carrying over  mh+mozilla: review+

Hi,

> You could remove the enclosing parentheses: #if defined(__GNUC__) && ...

Did so.

> Please remove the trailing whitespace.

Did so. I have no idea how I missed it...

Also, I looked hard and noticed an extra new line that crept in
and also an unwanted shifting of the keyword to the next line after the function name.
So I tightened them up.

OK to commit?
If so, I put checkin-needed keyword.
Attachment #8589748 - Attachment is obsolete: true
Attachment #8591111 - Flags: review+
(Assignee)

Comment 14

3 years ago
(In reply to ISHIKAWA, Chiaki from comment #13)
> Created attachment 8591111 [details] [diff] [review]
> Making mozalloc.h friendly to GCC 4.9 and address sanitizer build (Take 5)
> carrying over  mh+mozilla: review+
> 
> Hi,
> 
> > You could remove the enclosing parentheses: #if defined(__GNUC__) && ...
> 
> Did so.
> 
> > Please remove the trailing whitespace.
> 
> Did so. I have no idea how I missed it...
> 
> Also, I looked hard and noticed an extra new line that crept in
> and also an unwanted shifting of the keyword to the next line after the
> function name.
> So I tightened them up.
> 
> OK to commit?
> If so, I put checkin-needed keyword.

Oh well, I put checkin-needed keyword since it is a whitespace chagen.
(Also, I realized that if I put review flag as |+| carrying over mike's |+|, there was no way to ask for a review (of course, I could have used feedback, come to think of it.)

TIA


TIA
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/33e89c9a4172
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/33e89c9a4172
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.