Closed Bug 928808 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: octoploid, Assigned: glandium)

References

(Blocks 2 open bugs, )

Details

Attachments

(4 files, 2 obsolete files)

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
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?
(In reply to Octoploid from comment #1)
> Does this look reasonable?

Not at all.
(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.
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.
Attached patch Patch (v1)Splinter Review
(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.
Assignee: nobody → ekr
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)
Attached patch WIP for TURN TCP (obsolete) — Splinter Review
Added
Assignee: nobody → ekr
Attachment #820780 - Attachment is obsolete: true
Assignee: ekr → nobody
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)
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
(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))?
(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.
(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.
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.
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?
(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]
(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)
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?
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)
(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)
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)
Attachment #8374347 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/bc69628e2ad1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
This happens again on my mbp...
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.
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.