635790 - Depends on gnu89 inline

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: 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

[(no longer active)]

bsmedberg might be the right person to review this.

Comment 2

[Paul Biggar]

I can review this if bsmedberg is busy.

Comment 3

[(no longer active)]

Comment on attachment 514090 [details] [diff] [review]
proposed patch

He's always busy!

Comment 4

[Paul Biggar]

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)]

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)]

Attachment: 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)]

A try run with this patch:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=9dff02eb5cdd

Comment 8

[Paul Biggar]

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]

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)]

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]

(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)]

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]

(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)]

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]

(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)]

Attachment: 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]

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)]

Attachment: Dump of dll_obj/jemalloc.obj

Comment 19

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: dump of mt_obj/jemalloc.obj

Comment 20

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

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)]

Attachment: more comments

Comment 22

[Paul Biggar]

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)]

Attachment: patch with more comments

Comment 24

[Paul Biggar]

Comment on attachment 528401 [details] [diff] [review]
patch with more comments

Good job, thanks for fixing this up.

Comment 25

[(no longer active)]

Mounir, can you please take care of landing this?

Comment 26

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/1e64d699aa01