Enable GCGlobalNew.cpp for sunstudio compiler.

VERIFIED FIXED

Status

Tamarin
Garbage Collection (mmGC)
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: Leon Sha, Assigned: Leon Sha)

Tracking

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
Sunstudio is the default complier on solaris. Sunstudio do not support the always inline feature -- "inline __attribute__((always_inline)) ". So we should enable GCGlobalNew.cpp when using sunstudio. Aslo __hidden is required to avoid conflict with system new.
(Assignee)

Comment 1

11 years ago
Created attachment 298895 [details] [diff] [review]
patch
Attachment #298895 - Flags: review?(treilly)

Comment 2

11 years ago
I don't see anything in this patch about always_inline, just __hidden.

Does Sunstudio have *any* way to force a function inline?
(Assignee)

Comment 3

11 years ago
(In reply to comment #2)
> I don't see anything in this patch about always_inline, just __hidden.
> 
> Does Sunstudio have *any* way to force a function inline?
> 
We will not inline "new" when built with sunstudio. We use __hidden to make sure that we call our "new" instead of system "new". always_inline is in GCGlobalNew.h.
There is no way to force a function inline in sunstudio. You can only suggest sunstudio to inline a function, but sunstudio does not guarantee the function is always inlined.

Also I found the OVERRIDE_GLOBAL_NEW is not defined, while in flash player it is defined. Should I also add it into linuxbuild.h and solarisbuild.h?
(Assignee)

Updated

11 years ago
Attachment #298895 - Flags: review?(treilly) → review?(stejohns)
(Assignee)

Comment 4

11 years ago
Created attachment 300842 [details] [diff] [review]
patch v2

Add define OVERRIDE_GLOBAL_NEW for linux and solaris.
Attachment #298895 - Attachment is obsolete: true
Attachment #300842 - Flags: review?(stejohns)
Attachment #298895 - Flags: review?(stejohns)

Comment 5

10 years ago
Once this patch is pushed I will enable the buildslave for solaris bits.

Comment 6

10 years ago
Looks good, except I'm unclear as to why  	

#define OVERRIDE_GLOBAL_NEW

was added for Linux as well as Solaris.
(Assignee)

Comment 7

10 years ago
I have checked the flash player souce. When building flash player, OVERRIDE_GLOBAL_NEW was always added as a compiler option for linux. I thought it should be a default setting. If it is not, I need to redo the patch.
(In reply to comment #6)
> Looks good, except I'm unclear as to why        
> 
> #define OVERRIDE_GLOBAL_NEW
> 
> was added for Linux as well as Solaris.
> 

Comment 8

10 years ago
Ah, ok then. Sounds good.

Updated

10 years ago
Attachment #300842 - Flags: review?(stejohns) → review+
(Assignee)

Comment 9

10 years ago
Pushed.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Heh--that OVERRIDE_GLOBAL_NEW broke ActionMonkey on Linux.  (Solaris too, probably, fwiw.)

This is because some Mozilla source files are including MMgc.h, indirectly, but don't use it (not intentionally, anyway) and thus aren't linked against it.  Bug 427828.
Leon, can we revert the OVERRIDE_GLOBAL_NEW change and reopen this bug?  Flash Player isn't the only application using MMgc anymore.  I think it makes sense for the default setting to be the same on all platforms (especially since there's no way to override this and turn it off).
(Assignee)

Comment 12

10 years ago
Created attachment 314484 [details] [diff] [review]
Don't enable global new as default

We give this choice to the application who use MMGC.
Attachment #314484 - Flags: review?(stejohns)
(Assignee)

Updated

10 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 13

10 years ago
Comment on attachment 314484 [details] [diff] [review]
Don't enable global new as default

okie dokie
Attachment #314484 - Flags: review?(stejohns) → review+
(Assignee)

Comment 14

10 years ago
Pushed in.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Comment 15

9 years ago
Resolved fixed engineering / work item that has been pushed.  Setting status to verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.