Closed Bug 413804 Opened 16 years ago Closed 16 years ago

Enable GCGlobalNew.cpp for sunstudio compiler.

Categories

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

x86
SunOS
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: leon.sha, Assigned: leon.sha)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch patch (obsolete) — Splinter Review
Attachment #298895 - Flags: review?(treilly)
I don't see anything in this patch about always_inline, just __hidden.

Does Sunstudio have *any* way to force a function inline?
(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?
Attachment #298895 - Flags: review?(treilly) → review?(stejohns)
Attached patch patch v2Splinter Review
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)
Once this patch is pushed I will enable the buildslave for solaris bits.
Looks good, except I'm unclear as to why  	

#define OVERRIDE_GLOBAL_NEW

was added for Linux as well as Solaris.
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.
> 

Ah, ok then. Sounds good.
Attachment #300842 - Flags: review?(stejohns) → review+
Pushed.
Status: NEW → RESOLVED
Closed: 16 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).
We give this choice to the application who use MMGC.
Attachment #314484 - Flags: review?(stejohns)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 314484 [details] [diff] [review]
Don't enable global new as default

okie dokie
Attachment #314484 - Flags: review?(stejohns) → review+
Pushed in.
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
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.

Attachment

General

Creator:
Created:
Updated:
Size: