If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla30

Status

()

Core
General
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: Octoploid, Assigned: glandium)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla30
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
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?
(Assignee)

Comment 2

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

Not at all.
(Reporter)

Comment 3

4 years ago
(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.
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.
(Reporter)

Comment 5

4 years ago
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.
Created attachment 820495 [details] [diff] [review]
Patch (v1)
(Reporter)

Comment 7

4 years ago
(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

4 years ago
Created attachment 820593 [details] [diff] [review]
Move operator new etc. from .h to .cpp and make not-inline

Updated

4 years ago
Assignee: nobody → ekr

Comment 9

4 years ago
The patch in c8 compiles and links. I do not claim that it is ideal, but it should get people unwedged.
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.
Assignee: ekr → nobody
(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.  :-)
Flags: needinfo?(mh+mozilla)
Created attachment 820780 [details] [diff] [review]
WIP for TURN TCP

Added

Updated

4 years ago
Assignee: nobody → ekr

Updated

4 years ago
Attachment #820780 - Attachment is obsolete: true

Updated

4 years ago
Assignee: ekr → nobody

Updated

4 years ago
Blocks: 863846
(Assignee)

Comment 13

4 years ago
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.
Assignee: nobody → mh+mozilla
Flags: needinfo?(mh+mozilla)

Comment 14

4 years ago
This bit me with clang 3.4 from mac ports this week. :(
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 16

4 years ago
"Fixed" in Clang. See http://llvm.org/bugs/show_bug.cgi?id=17591 for details.
(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))?
(Reporter)

Comment 18

4 years ago
(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.
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.
Blocks: 929478
(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.
(Reporter)

Comment 21

4 years ago
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

4 years ago
Created attachment 8340685 [details] [diff] [review]
Patch we use in pkgsrc (from Joerg Sonnenberger)
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?
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)
Attachment #8365643 - Attachment description: hackaround → hackaround: turn off Winline-new-delete in configure.in, for compilers that recognize it
Summary: Clang trunk: mozalloc error: replacement function 'operator new' cannot be declared 'inline' → Clang trunk: mozalloc error: replacement function 'operator new' cannot be declared 'inline' [-Winline-new-delete]
(Reporter)

Comment 25

4 years ago
(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.)
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!
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 27

4 years ago
AIUI, there's nothing to do here, except maybe hiding the warning, since clang doesn't error out anymore.
Flags: needinfo?(mh+mozilla)
(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

4 years ago
Perhaps -Wno-error=inline-new-delete?
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)
(Assignee)

Comment 31

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

Yes
OK; I'll plan on tweaking the code-comment in that patch to be more useful/informative, & I'll repost it for review soon.
Flags: needinfo?(dholbert)
Created attachment 8374347 [details] [diff] [review]
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.
Attachment #8365643 - Attachment is obsolete: true
Attachment #8374347 - Flags: review?(mh+mozilla)
Flags: needinfo?(dholbert)
(Assignee)

Updated

4 years ago
Attachment #8374347 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc69628e2ad1
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/bc69628e2ad1
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
This happens again on my mbp...
Yup, see bug 1155393.
Blocks: 1155393
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
I filed bug 1261719 for the Visual Studio 2015 Update 2 case.

Comment 40

a year ago
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 ----
You need to log in before you can comment on or make changes to this bug.