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.
I can review this if bsmedberg is busy.
Comment on attachment 514090 [details] [diff] [review] proposed patch He's always busy!
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.
A try run with this patch: http://tbpl.mozilla.org/?tree=MozillaTry&rev=9dff02eb5cdd
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 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__?
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.
(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.
(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?
(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.
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.
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
Comment on attachment 528116 [details] [diff] [review] more comments Great, thanks. Can you mention the section of that doc? r+ with that change.
Created attachment 528401 [details] [diff] [review] patch with more comments
6 years ago
Comment on attachment 528401 [details] [diff] [review] patch with more comments Good job, thanks for fixing this up.
Mounir, can you please take care of landing this?