The default bug view has changed. See this FAQ.

Depends on gnu89 inline

RESOLVED FIXED in mozilla6

Status

()

Core
Memory Allocator
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

unspecified
mozilla6
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

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
bsmedberg might be the right person to review this.
Attachment #514090 - Flags: review?(benjamin)

Comment 2

6 years ago
I can review this if bsmedberg is busy.
Comment on attachment 514090 [details] [diff] [review]
proposed patch

He's always busy!
Attachment #514090 - Flags: review?(benjamin) → review?(pbiggar)

Updated

6 years ago
Assignee: nobody → respindola

Comment 4

6 years ago
Is there a way to make it inline in c99 like gnu89 does? It pretty clearly wants to be inlines later in the file.
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.
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.
Attachment #514090 - Attachment is obsolete: true
Attachment #514090 - Flags: review?(pbiggar)
Attachment #524057 - Flags: review?
A try run with this patch:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=9dff02eb5cdd

Comment 8

6 years ago
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).
Attachment #524057 - Flags: review? → review?(pbiggar)

Comment 9

6 years ago
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__?
Attachment #524057 - Flags: review?(pbiggar) → review-
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

6 years ago
(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?
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

6 years ago
(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?
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

6 years ago
(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).
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.
Attachment #524057 - Attachment is obsolete: true
Attachment #527782 - Flags: review?(pbiggar)

Comment 17

6 years ago
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.
Attachment #527782 - Flags: review?(pbiggar) → review+
Created attachment 528108 [details]
Dump of dll_obj/jemalloc.obj
Created attachment 528109 [details]
dump of mt_obj/jemalloc.obj
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.
Created attachment 528116 [details] [diff] [review]
more comments
Attachment #527782 - Attachment is obsolete: true
Attachment #528116 - Flags: review?(pbiggar)

Comment 22

6 years ago
Comment on attachment 528116 [details] [diff] [review]
more comments

Great, thanks.

Can you mention the section of that doc?

r+ with that change.
Attachment #528116 - Flags: review?(pbiggar) → review+
Created attachment 528401 [details] [diff] [review]
patch with more comments
Attachment #528116 - Attachment is obsolete: true
Attachment #528401 - Flags: review?(pbiggar)
Keywords: checkin-needed

Comment 24

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

Good job, thanks for fixing this up.
Attachment #528401 - Flags: review?(pbiggar) → review+
Mounir, can you please take care of landing this?
http://hg.mozilla.org/mozilla-central/rev/1e64d699aa01
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
You need to log in before you can comment on or make changes to this bug.