928808 - Clang trunk: mozalloc error: replacement function 'operator new' cannot be declared 'inline' [-Winline-new-delete]

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Octoploid]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20131020165230

Steps to reproduce:

Try to build Firefox with Clang trunk.


Actual results:

In file included from /home/markus/mozilla-central/memory/mozalloc/mozalloc.cpp:29:
../../dist/include/mozilla/mozalloc.h:198:21: error: replacement function 'operator new' cannot be declared 'inline'
MOZALLOC_EXPORT_NEW MOZALLOC_INLINE
                    ^
../../dist/include/mozilla/mozalloc.h:44:27: note: expanded from macro 'MOZALLOC_INLINE'
#  define MOZALLOC_INLINE MOZ_ALWAYS_INLINE_EVEN_DEBUG
                          ^
../../dist/include/mozilla/Attributes.h:43:75: note: expanded from macro 'MOZ_ALWAYS_INLINE_EVEN_DEBUG'
#  define MOZ_ALWAYS_INLINE_EVEN_DEBUG     __attribute__((always_inline)) MOZ_INLINE
                                                                          ^
../../dist/include/mozilla/Attributes.h:21:33: note: expanded from macro 'MOZ_INLINE'
#  define MOZ_INLINE            inline
                                ^
In file included from /home/markus/mozilla-central/memory/mozalloc/mozalloc.cpp:29:
../../dist/include/mozilla/mozalloc.h:204:21: error: replacement function 'operator new' cannot be declared 'inline'
MOZALLOC_EXPORT_NEW MOZALLOC_INLINE
                    ^
../../dist/include/mozilla/mozalloc.h:44:27: note: expanded from macro 'MOZALLOC_INLINE'
#  define MOZALLOC_INLINE MOZ_ALWAYS_INLINE_EVEN_DEBUG
                          ^
../../dist/include/mozilla/Attributes.h:43:75: note: expanded from macro 'MOZ_ALWAYS_INLINE_EVEN_DEBUG'
#  define MOZ_ALWAYS_INLINE_EVEN_DEBUG     __attribute__((always_inline)) MOZ_INLINE
                                                                          ^
../../dist/include/mozilla/Attributes.h:21:33: note: expanded from macro 'MOZ_INLINE'
#  define MOZ_INLINE            inline
                                ^
In file included from /home/markus/mozilla-central/memory/mozalloc/mozalloc.cpp:29:
...


Expected results:

See: http://cplusplus.github.io/LWG/lwg-active.html#2340

Comment 1

[Octoploid]

Moving the definitions from mozalloc.h to memory/mozalloc/mozalloc.cpp
(just leaving the declarations) and un-inline them all, seems to fix the issue.
Does this look reasonable?

Comment 2

[Mike Hommey [:glandium]]

(In reply to Octoploid from comment #1)
> Does this look reasonable?

Not at all.

Comment 3

[Octoploid]

(In reply to Mike Hommey [:glandium] from comment #2)
> (In reply to Octoploid from comment #1)
> > Does this look reasonable?
> 
> Not at all.

You don't say so.

Comment 4

[(no longer active)]

This is the clang commit which added this check: <https://github.com/llvm-mirror/clang/commit/d2b0cf38>

It seems like they're right indeed, and our code is not C++11.

Comment 5

[Octoploid]

17.6.4.6 Replacement functions
 [replacement.functions]
 Clauses 18 through 30 and Annex D describe the behavior of numerous functions defined by the C ++ standard
library. Under some circumstances, however, certain of these function descriptions also apply to replacement
functions defined in the program (17.3).
 A C ++ program may provide the definition for any of eight dynamic memory allocation function signatures
declared in header <new> (3.7.4, 18.6):
— operator new(std::size_t)
— operator new(std::size_t, const std::nothrow_t&)
— operator new[](std::size_t)
— operator new[](std::size_t, const std::nothrow_t&)
— operator delete(void*)
— operator delete(void*, const std::nothrow_t&)
— operator delete[](void*)
— operator delete[](void*, const std::nothrow_t&)
The program’s definitions are used instead of the default versions supplied by the implementation (18.6).
Such replacement occurs prior to program startup (3.2, 3.6). The program’s definitions shall not be specified
as inline. No diagnostic is required.

Comment 6

[(no longer active)]

Attachment: Patch (v1)

Comment 7

[Octoploid]

(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #6)
> Created attachment 820495 [details] [diff] [review]
> Patch (v1)

Your patch won't work, because you'll get link errors later on.

Comment 8

[Eric Rescorla (:ekr)]

Attachment: Move operator new etc. from .h to .cpp and make not-inline

Comment 9

[Eric Rescorla (:ekr)]

The patch in c8 compiles and links. I do not claim that it is ideal, but it should get people unwedged.

Comment 10

[Eric Rescorla (:ekr)]

De-assigning this from me, since glandium explicitly says my approach in c8 is the wrong thing. Sounds like we need someone to figure out what an acceptable fix is and that's probably not me.

Comment 11

[(no longer active)]

(In reply to Eric Rescorla (:ekr) from comment #10)
> De-assigning this from me, since glandium explicitly says my approach in c8
> is the wrong thing. Sounds like we need someone to figure out what an
> acceptable fix is and that's probably not me.

That would be glandium.  :-)

Comment 12

[Eric Rescorla (:ekr)]

Attachment: WIP for TURN TCP

Added

Comment 13

[Mike Hommey [:glandium]]

I'm out of short term workarounds. I'm afraid we'll have to go the hard way, which is to merge mozalloc in mozglue. That was on my todo list, and i have a half-year old, half working patch. IIRC the main issue was with windows. If that's really the case, maybe i can come up with something that keeps it on windows for now until those subtle details are figured.

Comment 14

[Bob Clary [:bc] (inactive)]

This bit me with clang 3.4 from mac ports this week. :(

Comment 15

[Christian Holler (:decoder)]

Fwiw, David Majnemer told me in the #llvm IRC channel that this has nothing to do with C++11:

<majneme__> Mozilla shouldn't have those things inline, nasty things can happen
<majneme__> It is an extension of the following defect report: http://cplusplus.github.io/LWG/lwg-defects.html#404
<majneme__> That LWG DR applies to C++03 too

At least Clang doesn't support any standard where this would be valid to do. Not sure if it affects the way we want to fix this.

Comment 16

[Octoploid]

"Fixed" in Clang. See http://llvm.org/bugs/show_bug.cgi?id=17591 for details.

Comment 17

[(no longer active)]

(In reply to comment #16)
> "Fixed" in Clang. See http://llvm.org/bugs/show_bug.cgi?id=17591 for details.

So should we mark these functions as __attribute__((used))?

Comment 18

[Octoploid]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #17)
> (In reply to comment #16)
> > "Fixed" in Clang. See http://llvm.org/bugs/show_bug.cgi?id=17591 for details.
> 
> So should we mark these functions as __attribute__((used))?

I think this blows up the object size unnecessarily. E.g. peak memory use
of the final libxul link during a clang LTO build went from 4.8 GB to 5.3 GB.
So maybe simply adding -Wno-inline-new-delete to CXXFLAGS would be better, 
because the address of operator new/delete is taken nowhere AFAICS.

Comment 19

[Christian Holler (:decoder)]

What's the status on this? It would be nice if we could use Clang tip again, especially for the Sanitizers. TSan for example is suffering some severe issues in older versions.

Comment 20

[(no longer active)]

(In reply to comment #18)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #17)
> > (In reply to comment #16)
> > > "Fixed" in Clang. See http://llvm.org/bugs/show_bug.cgi?id=17591 for details.
> > 
> > So should we mark these functions as __attribute__((used))?
> 
> I think this blows up the object size unnecessarily. E.g. peak memory use
> of the final libxul link during a clang LTO build went from 4.8 GB to 5.3 GB.
> So maybe simply adding -Wno-inline-new-delete to CXXFLAGS would be better, 
> because the address of operator new/delete is taken nowhere AFAICS.

That sounds good to me.

Comment 21

[Octoploid]

I don't know the build machinery good enough, but
checking for (__clang_major__ * 10 + __clang_minor__ >= 34) and 
adding -Wno-inline-new-delete in this case should be enough.

(In reply to Christian Holler (:decoder) from comment #19)
> What's the status on this? It would be nice if we could use Clang tip again,
> especially for the Sanitizers. TSan for example is suffering some severe
> issues in older versions.

You could use Clang tip without problems. You will just get many warnings if 
-Wno-inline-new-delete isn't set.

Comment 22

[Martin Husemann]

Attachment: Patch we use in pkgsrc (from Joerg Sonnenberger)

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

I just hit this with Clang 3.4, with --enable-warnings-as-errors in my mozconfig.

Should I stop using -enable-warnings-as-errors, or avoid using clang 3.4 for now?

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: hackaround: turn off Winline-new-delete in configure.in, for compilers that recognize it

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #20)
> > So maybe simply adding -Wno-inline-new-delete to CXXFLAGS would be better, 
> > because the address of operator new/delete is taken nowhere AFAICS.
> 
> That sounds good to me.

Here's a hackaround patch that does this, FWIW. (not necessarily posting as an actual fix; more as a local workaround for folks to use if they like)

Someone who understands the issues here better than I do: feel free to take this, improve the comment to make it meaningful, and request review on it. (or alternately, suggest some explanatory text in a bug-comment here, and I can incorporate it into the patch & request review myself)

Comment 25

[Octoploid]

(In reply to Daniel Holbert [:dholbert] from comment #24)
> Created attachment 8365643 [details] [diff] [review]
> hackaround: turn off Winline-new-delete in configure.in, for compilers that
> recognize it
> 
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #20)
> > > So maybe simply adding -Wno-inline-new-delete to CXXFLAGS would be better, 
> > > because the address of operator new/delete is taken nowhere AFAICS.
> > 
> > That sounds good to me.
> 
> Here's a hackaround patch that does this, FWIW. (not necessarily posting as
> an actual fix; more as a local workaround for folks to use if they like)
> 
> Someone who understands the issues here better than I do: feel free to take
> this, improve the comment to make it meaningful, and request review on it.
> (or alternately, suggest some explanatory text in a bug-comment here, and I
> can incorporate it into the patch & request review myself)

> we inline 'new' and 'delete' in mozalloc (XXXwhy?)

The only reason is performance. And because both gcc and clang accept inline replacement operators new+delete
there is no reason not to inline.  

BTW, because the standard library must accept replacement new+delete, some people even provide definitions in
their code that is roughly equivalent to the standard one, just so it could be inlined.

There are discussions to add a -D_GLIBCXX_FORCE_DEFAULT_ALLOCATION to libstdc++ in gcc, that would only provide
the inline versions. (Obviously replacement operators new+delete wouldn't work in this case.)

Comment 26

[(no longer active)]

Mike, how should we proceed here?  It seems like there are several ideas floating around on how to resolve this issue, and it would be nice if we took some solution here.  Thanks!

Comment 27

[Mike Hommey [:glandium]]

AIUI, there's nothing to do here, except maybe hiding the warning, since clang doesn't error out anymore.

Comment 28

[(no longer active)]

(In reply to comment #27)
> AIUI, there's nothing to do here, except maybe hiding the warning, since clang
> doesn't error out anymore.

It will if you treat warnings as errors!

So is attachment 8365643 [details] [diff] [review] the right fix?

Comment 29

[Alex Xu]

Perhaps -Wno-error=inline-new-delete?

Comment 30

[Daniel Holbert [:dholbert]]

That's effectively what attachment 8365643 [details] [diff] [review] does, behind some autoconf magic. (though -Wno instead of -Wno-error, because there's no point in even spamming a warning about this if we decide it's not fix-worthy)

Comment 31

[Mike Hommey [:glandium]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #28)
> So is attachment 8365643 [details] [diff] [review] the right fix?

Yes

Comment 32

[Daniel Holbert [:dholbert]]

OK; I'll plan on tweaking the code-comment in that patch to be more useful/informative, & I'll repost it for review soon.

Comment 33

[Daniel Holbert [:dholbert]]

Attachment: Patch: turn off Winline-new-delete in configure.in, for compilers that recognize it

OK, here's the same hackaround patch, but now with a more useful code-comment.

Comment 34

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc69628e2ad1

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bc69628e2ad1

Comment 36

[Xidorn Quan [:xidorn] UTC+11]

This happens again on my mbp...

Comment 37

[Daniel Holbert [:dholbert]]

Yup, see bug 1155393.

Comment 38

[Chris Pearce [:cpearce (Not reading bugmail)]]

We are now hitting the equivalent warning with VisualStudio 2015 Pro Update 2:

 1:53.47 In the directory  /c/Users/cpearce/src/central/objdir/dom/cellbroadcast
 1:53.47 The following command failed to execute properly:
 1:53.47 c:/Users/cpearce/src/central/objdir/_virtualenv/Scripts/python.exe -m mozbuild.action.cl cl -FoUnified_cpp_dom_cellbroadcast0.obj -c -Ic:/Users/cpearce/src/central/objdir/dist/stl_wrappers -DDEBUG=1 -DTRACING=1 -DWIN32_LEAN_AND_MEAN -D_WIN32 -DWIN32 -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DOS_WIN=1 -D_UNICODE -DCHROMIUM_BUILD -DU_STATIC_IMPLEMENTATION -DUNICODE -D_WINDOWS -D_SECURE_ATL -DCOMPILER_MSVC -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -Ic:/Users/cpearce/src/central/dom/cellbroadcast -Ic:/Users/cpearce/src/central/objdir/dom/cellbroadcast -Ic:/Users/cpearce/src/central/objdir/ipc/ipdl/_ipdlheaders -Ic:/Users/cpearce/src/central/ipc/chromium/src -Ic:/Users/cpearce/src/central/ipc/glue -Ic:/Users/cpearce/src/central/objdir/dist/include -Ic:/Users/cpearce/src/central/objdir/dist/include/nspr -Ic:/Users/cpearce/src/central/objdir/dist/include/nss -MD -FI c:/Users/cpearce/src/central/objdir/mozilla-config.h -DMOZILLA_CLIENT -TP -nologo -wd5026 -wd5027 -Zc:sizedDealloc- -Zc:threadSafeInit- -wd4091 -D_HAS_EXCEPTIONS=0 -W3 -Gy -arch:IA32 -FS -wd4251 -wd4244 -wd4267 -wd4345 -wd4351 -wd4800 -wd4819 -we4553 -GR- -Zi -O1 -Oi -Oy- -WX -Fdgenerated.pdb c:/Users/cpearce/src/central/objdir/dom/cellbroadcast/Unified_cpp_dom_cellbroadcast0.cpp
 1:53.47 c:/Users/cpearce/src/central/config/rules.mk:918: recipe for target 'Unified_cpp_dom_cellbroadcast0.obj' failed
 1:53.47 mozmake.EXE[5]: *** [Unified_cpp_dom_cellbroadcast0.obj] Error 1
 1:53.47 c:/Users/cpearce/src/central/config/recurse.mk:71: recipe for target 'dom/cellbroadcast/target' failed
 1:53.47 mozmake.EXE[4]: *** [dom/cellbroadcast/target] Error 2
 1:56.27 Unified_cpp_dom_presentation0.cpp
 1:56.27 c:\users\cpearce\src\central\objdir\dist\include\mozilla/mozalloc.h(184): error C2220: warning treated as error - no 'object' file generated
 1:56.27 Warning: C4595 in c:\users\cpearce\src\central\objdir\dist\include\mozilla\mozalloc.h: 'operator new': non-member operator new or delete functions may not be declared inline
 1:56.27 c:\users\cpearce\src\central\objdir\dist\include\mozilla/mozalloc.h(184): warning C4595: 'operator new': non-member operator new or delete functions may not be declared inline
 1:56.27 c:\users\cpearce\src\central\objdir\dist\include\mozilla/mozalloc.h(184): note: to simplify migration, consider the temporary use of /Wv:18 flag with the version of the compiler with which you used to build without warnings

Comment 39

[Chris Pearce [:cpearce (Not reading bugmail)]]

I filed bug 1261719 for the Visual Studio 2015 Update 2 case.

Comment 40

[altblitz]

Firefox rv:48.0 "Patch: turn off Winline-new-delete in configure.in, for compilers that recognize it" is not present in old-configure.in. 
"MOZ_CXX_SUPPORTS_WARNING(-Wno-, inline-new-delete, ac_cxx_has_wno_inline_new_delete)"

---- begin cut ----
In file included from /var/spool/makepkg/firefox/src/firefox-48.0/toolkit/library/StaticXULComponentsStart.cpp:1:
In file included from /var/spool/makepkg/firefox/src/firefox-48.0/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Module.h:10:
In file included from /var/spool/makepkg/firefox/src/firefox-48.0/obj-x86_64-pc-linux-gnu/dist/include/nscore.h:20:
/var/spool/makepkg/firefox/src/firefox-48.0/obj-x86_64-pc-linux-gnu/dist/include/mozilla/mozalloc.h:225:21: warning: replacement function 'operator delete[]' cannot be declared 'inline' [-Winline-new-delete]
MOZALLOC_EXPORT_NEW MOZALLOC_INLINE
                    ^
/var/spool/makepkg/firefox/src/firefox-48.0/obj-x86_64-pc-linux-gnu/dist/include/mozilla/mozalloc.h:31:27: note: expanded from macro 'MOZALLOC_INLINE'
#  define MOZALLOC_INLINE MOZ_ALWAYS_INLINE_EVEN_DEBUG
                          ^
/var/spool/makepkg/firefox/src/firefox-48.0/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Attributes.h:27:75: note: expanded from macro 'MOZ_ALWAYS_INLINE_EVEN_DEBUG'
#  define MOZ_ALWAYS_INLINE_EVEN_DEBUG     __attribute__((always_inline)) inline
                                                                          ^
8 warnings generated.
libxul_s.a.desc
libxul.so
---- end cut ----