Last Comment Bug 635790 - Depends on gnu89 inline
: Depends on gnu89 inline
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Memory Allocator (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla6
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
Mentors:
Depends on:
Blocks: clang
  Show dependency treegraph
 
Reported: 2011-02-21 16:16 PST by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2011-04-27 03:12 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (472 bytes, patch)
2011-02-21 16:16 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Review
use aliases (2.70 KB, patch)
2011-04-05 07:22 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
paul.biggar: review-
Details | Diff | Review
patch with updated comments (3.31 KB, patch)
2011-04-22 09:08 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
paul.biggar: review+
Details | Diff | Review
Dump of dll_obj/jemalloc.obj (433.09 KB, text/plain)
2011-04-25 08:49 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details
dump of mt_obj/jemalloc.obj (433.09 KB, text/plain)
2011-04-25 08:49 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details
more comments (3.49 KB, patch)
2011-04-25 09:12 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
paul.biggar: review+
Details | Diff | Review
patch with more comments (3.54 KB, patch)
2011-04-26 12:48 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
paul.biggar: review+
Details | Diff | Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-02-21 16:16:15 PST
Created attachment 514090 [details] [diff] [review]
proposed patch

In memory/jemalloc/jemalloc.c the function memalign in declared inline, but it is expected that it will be available externally. That is the gnu89 behavior but not what the c99 standard says. 

For more information see
http://clang.llvm.org/compatibility.html#inline
Comment 1 :Ehsan Akhgari (busy, don't ask for review please) 2011-03-23 11:17:04 PDT
bsmedberg might be the right person to review this.
Comment 2 Paul Biggar 2011-03-23 16:37:48 PDT
I can review this if bsmedberg is busy.
Comment 3 :Ehsan Akhgari (busy, don't ask for review please) 2011-03-23 16:52:02 PDT
Comment on attachment 514090 [details] [diff] [review]
proposed patch

He's always busy!
Comment 4 Paul Biggar 2011-03-23 17:05:12 PDT
Is there a way to make it inline in c99 like gnu89 does? It pretty clearly wants to be inlines later in the file.
Comment 5 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-03-23 19:34:34 PDT
We can compile the file in gnu89 mode if we really must, but it looks like the function is normally inlined, I only found the problem in a debug build.

I am leaving on vacations, but should be able to test that some time during next week.
Comment 6 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-04-05 07:22:13 PDT
Created attachment 524057 [details] [diff] [review]
use aliases

Ah, the joys of ELF's visibility rules. That is why the inline was there. On ELF memcpy can be changed at runtime and cannot normally be inlined.

A common way to handle this is to use aliases (http://www.akkadia.org/drepper/dsohowto.pdf).

The attached patch does it. I tested that on ELF (linux), gcc will produce tail calls to memalign_internal, which is probably better than inlining.
Comment 7 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-04-05 10:12:32 PDT
A try run with this patch:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=9dff02eb5cdd
Comment 8 Paul Biggar 2011-04-05 21:59:08 PDT
Comment on attachment 524057 [details] [diff] [review]
use aliases

I need to read the docs for these attributes. I plan to get this reviewed by the weekend.

(btw, r? without a target will almost universally get missed. I had to go hunting for this since it didnt show up in my dashboard).
Comment 9 Paul Biggar 2011-04-15 09:44:49 PDT
Comment on attachment 524057 [details] [diff] [review]
use aliases

(In reply to comment #6)
> Created attachment 524057 [details] [diff] [review]
> use aliases

Can you explain to me the objective of this patch? I think it's to fix a visibility rule while still maintaining the performance of inlining, on a GCC path. So, how does clang fit it? Which path does it take? Why have you removed the inline on non-GCC paths?

If I understand this correctly, with GCC, memalign_internal is hidden, and memalign is visible. This means that memalign is a symbol that can be linked to, and memalign_internal is hidden and therefore can be inlined. No, it can be tail-called to. Why can't it be inlined? Is that important? The DSO doc douesn't discuss it.

This needs loads of documentation, this is really obscure.

Can you clarify what happens in the case of Solaris+__GNUC__?
Comment 10 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-04-15 11:18:16 PDT
The main objective of the patch is to get this to build with clang. The problem of the code as it is right now is that it assumes that "inline" is the gun89 one.

Just removing the inline would be sufficient, but it forces gcc to use a PLT for calling memalign.

Using the alias makes gcc able to optimize the call to memalign_internal again. It can inline, specialize it, etc. In this particular case, the gcc I have (I think it was Fedora's 4.5) decided to tail call instead of inline.

Solaris uses ELF, so it should behave as Linux.
Comment 11 Paul Biggar 2011-04-16 05:12:26 PDT
(In reply to comment #10)

I had a lot of questions in comment 9. I'd really need to understand the answers to them before approving a patch like this, in particular:

> So, how does clang fit it? Which path does it take? Why have you removed
the inline on non-GCC paths?
Comment 12 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-04-16 07:38:39 PDT
Clang sees the same code as gcc (it defines __GNUC__). Is that what you mean by "fit in"?

That inline keyword depended on GCC semantics, but I can add it back for the non __SUNPRO_C non __GNU_C__ case if you want.
Comment 13 Paul Biggar 2011-04-21 11:58:44 PDT
(In reply to comment #12)
> Clang sees the same code as gcc (it defines __GNUC__). Is that what you mean by
> "fit in"?

Yes.
 

> That inline keyword depended on GCC semantics

Why do you say that? What does MSVC do?
Comment 14 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-04-21 15:12:02 PDT
See for example
http://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/C-Dialect-Options.html#C-Dialect-Options

GCC defaults to the gnu89 mode. It even has a -fgnu89-inline command line option if you want to use gnu89 inline functions in otherwise c99 mode.

I am not sure what semantics VC has for inline.

Would you be OK with the patch if I added back the inline keyword for the non __SUNPRO_C non __GNU_C__ case?
Comment 15 Paul Biggar 2011-04-22 07:35:12 PDT
(In reply to comment #14)
> I am not sure what semantics VC has for inline.

Which is why we can't just remove it.

> Would you be OK with the patch if I added back the inline keyword for the non
> __SUNPRO_C non __GNU_C__ case?

Yes. However, it also needs a big ol' comment. (This isn't an r+, please request a review again with the fixed patch - thanks).
Comment 16 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-04-22 09:08:11 PDT
Created attachment 527782 [details] [diff] [review]
patch with updated comments

http://tbpl.mozilla.org/?tree=MozillaTry&rev=a7400e87073a

I will update the bug once the VC results are in.
Comment 17 Paul Biggar 2011-04-22 09:44:57 PDT
Comment on attachment 527782 [details] [diff] [review]
patch with updated comments

I think the comment needs to explain that the internal version can be optimized (this is only alluded to), and explain what aliasing means, which is pretty obscure. It also needs a reference to the DSO doc (and section) that explains it, and possibly the gcc manual (if you think it's useful).

r+ with those changes, after checking the code MSVC generates with and without the inline.
Comment 18 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-04-25 08:49:17 PDT
Created attachment 528108 [details]
Dump of dll_obj/jemalloc.obj
Comment 19 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-04-25 08:49:48 PDT
Created attachment 528109 [details]
dump of mt_obj/jemalloc.obj
Comment 20 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-04-25 08:52:05 PDT
I have attached dumps of the two jemalloc.obj that I found in the windows build. The memalign is still being inlined.

I will update the comments on the patch and upload again.
Comment 21 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-04-25 09:12:40 PDT
Created attachment 528116 [details] [diff] [review]
more comments
Comment 22 Paul Biggar 2011-04-25 17:32:37 PDT
Comment on attachment 528116 [details] [diff] [review]
more comments

Great, thanks.

Can you mention the section of that doc?

r+ with that change.
Comment 23 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-04-26 12:48:10 PDT
Created attachment 528401 [details] [diff] [review]
patch with more comments
Comment 24 Paul Biggar 2011-04-26 13:20:26 PDT
Comment on attachment 528401 [details] [diff] [review]
patch with more comments

Good job, thanks for fixing this up.
Comment 25 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-26 15:39:27 PDT
Mounir, can you please take care of landing this?
Comment 26 Dão Gottwald [:dao] 2011-04-27 03:12:29 PDT
http://hg.mozilla.org/mozilla-central/rev/1e64d699aa01

Note You need to log in before you can comment on or make changes to this bug.