463629 - [redux] Non-const global data are verboten

Status: VERIFIED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Lars T Hansen]

We need to get rid of non-const global data in the VM core and in MMgc, this includes all non-const class-static variables.  Non-const global data are expensive or impossible on some platforms.

These types of data /are/ allowed in the AVM shell code.

Please report instances of non-const global data here, and attach patches here as well.

Comment 1

[Lars T Hansen]

MacOS command line build, JIT included, finds these obvious cases:

$ nm libavmplus.a | egrep ' (D|B) '

00009dc0 D __ZN7avmplus10digitTableE
00009f80 D __ZN7avmplus11letterTableE
00009d80 D __ZN7avmplus13extenderTableE
00009e00 D __ZN7avmplus18combiningCharTableE
00000be8 D __ZN7avmplus12GenericGuard13exceptionPortE
00000be4 D __ZN7avmplus12GenericGuard9guardListE
00000fa0 D __ZN7avmplus15WordcodeEmitter11transitionsE
00001060 D __ZN7avmplus15WordcodeEmitter6statesE
00000ce0 D __ZN7avmplus15WordcodeEmitter8toplevelE
00000024 D _pcre_callout
00000030 D _pcre_free
00000034 D _pcre_malloc
00000028 D _pcre_stack_free
0000002c D _pcre_stack_malloc
00000cf8 D __tprof_before_id

$ nm libMMgc.a | egrep ' (D|B) '

00000514 D __ZN4MMgc11FixedMalloc8instanceE
00000554 D __ZN4MMgc11GCHashtable7DELETEDE
00001b98 D __ZN4MMgc6GCHeap8instanceE

Comment 2

[Lars T Hansen]

Attachment: Patch for the tables in WordcodeEmitter and the unicode tables.

Comment 3

[Lars T Hansen]

Attachment: Patch for the globals in PCRE (compiles, not tested)

Comment 4

[Lars T Hansen]

(In reply to comment #1)

The globals in GenericGuard are JIT-specific and do not apply to platforms where globals might be an issue.

The variable _tprof_before_id is vprof-specific and is likely not an issue.

Comment 5

[Rick Reitmaier]

The only remaining user of GenericGuard is BufferGuard in AbcParser.  I wonder if its worth re-designing this usage.

As for the JIT, MIR used GrowthGuard (which pulls in GenericGuard) but NJ is not.  So once, we have platform parity for the JITs we can at least remove GrowthGuard.

Comment 6

[Rick Reitmaier]

Also the _tprof_before_id should only be compiled in for debug builds.  If this is landing in release builds, it needs to be removed.

Comment 7

[Steven Johnson]

Comment on attachment 346883 [details] [diff] [review]
Patch for the tables in WordcodeEmitter and the unicode tables.

the tables in AvmCore.cpp should probably be static as well as const, I don't think they are used elsewhere

Comment 8

[Steven Johnson]

Comment on attachment 346887 [details] [diff] [review]
Patch for the globals in PCRE (compiles, not tested)

Do we know if newer versions of PCRE improve this any?

Comment 9

[Steven Johnson]

(In reply to comment #5)
> The only remaining user of GenericGuard is BufferGuard in AbcParser.  I wonder
> if its worth re-designing this usage.

IMHO it is totally worth doing, as the existing approach isn't likely to work available on low-end hardware. 

> As for the JIT, MIR used GrowthGuard (which pulls in GenericGuard) but NJ is
> not.  So once, we have platform parity for the JITs we can at least remove
> GrowthGuard.

That will be a glorious day.

Comment 10

[Steven Johnson]

the Debugger also uses a few statics:

	static TraceLevel			astrace_console; 
	static TraceLevel			astrace_callback;
	static bool				in_trace;
	static uint64				astraceStartTime;

Comment 11

[Lars T Hansen]

Attachment: Patch for the globals in PCRE

Fixes a linking problem and makes sure that the files that include the PCRE header turn on the AVMPLUS-specific code.  (This feels like a hack but it's not obviously worse than anything else.)

Comment 12

[Lars T Hansen]

(In reply to comment #2)
> Created an attachment (id=346883) [details]
> Patch for the tables in WordcodeEmitter and the unicode tables.

Pushed: tamarin-redux changeset 1075:5a6e728e05ee

Comment 13

[Lars T Hansen]

(In reply to comment #11)
> Created an attachment (id=347285) [details]
> Patch for the globals in PCRE

Pushed: tamarin-redux changeset 1078:e56d4aeb5992

Comment 14

[Steven Johnson]

Actually, defining AVMPLUS_PCRE in config.h isn't a good fix after all -- this is only included internally to PCRE, but external clients rely on it as well (eg Flash need to call pcre_free directly, but since it doesn't include pcre/config.h prior to pcre.h, it tries to access the old, global-var definition). Plus, configure.h is autogenerated :-)

I suggest we just move the #define into pcre.h

Comment 15

[Steven Johnson]

Attachment: Patch

Move AVMPLUS_PCRE define to pcre.h 

(feel free to go ahead and push this yourself if you approve it)

Comment 16

[Lars T Hansen]

In that case we should remove the #define AVMPLUS_PCRE I added to some of the regex classes.  I'll take care of that and and push the fix.

Comment 17

[Lars T Hansen]

Two more work items:

* the global new/delete operators can't be used; at a minimum they must take a parameter giving them some heap structure to allocate out of (since they can't pick that out of a global variable).  It's possible we need new/delete that take the AvmCore as a parameter.

* the pointer tables for builtins need to be const, but more than that it may not be possible for them to be global constants on all platforms.  It may be necessary to hide them in macrology so that they're constants when possible but runtime-initialized data structures otherwise.  Then the question becomes, off of what do we hang these data structures?

Comment 18

[Lars T Hansen]

(In reply to comment #15)
> Created an attachment (id=347409) [details]
> Patch
> 
> Move AVMPLUS_PCRE define to pcre.h 

Pushed: tamarin-redux changeset 1085:54fffe5dc5dc

Comment 19

[Edwin Smith]

now that it is possible to generate pointer tables automatically, and doing so also requires calling a [possibly macroized] [possibly generated] function, I would suggest PoolObject as the place to hang such structures.  initializing them can be done with macroology or with generated code, if we add a flag to nativegen.py

Comment 20

[Lars T Hansen]

Attachment: Patch to remove the MMgc global variables

This patch gets rid of most use of global variables in MMgc, including dependencies on the global new and delete operators, as follows.

A new #define, MMGC_GLOBAL_VARIABLES, is introduced.  This will not be defined on platforms that don't allow the use of global variables.

The existing global variables, GCHeap::instance and FixedMalloc::instance, and methods operating on those are available only when the #define is enabled.  Most code is rewritten so as not to depend on MMGC_GLOBAL_VARIABLES being on, the MEMORY_INFO subsystem being the main exception.  This means that the global FixedMalloc and GCHeap instances are available through other data structures by accessor methods.

Most uses of the global new and delete operators are rewritten as uses of AvmCore::fixedAlloc and AvmCore::fixedFree, which allocate via the system-wide FixedMalloc instance.

Comment 21

[Steven Johnson]

(In reply to comment #19)
> now that it is possible to generate pointer tables automatically, and doing so
> also requires calling a [possibly macroized] [possibly generated] function, I
> would suggest PoolObject as the place to hang such structures.  initializing
> them can be done with macroology or with generated code, if we add a flag to
> nativegen.py

Definitely doable, but may require a large amount of churn in existing native-glue. But, it's all modest and can be done in baby steps... let me start thinking on it.

Comment 22

[Lars T Hansen]

(In reply to comment #20)
> Created an attachment (id=347538) [details]
> Patch to remove the MMgc global variables

This patch is incomplete and should be considered preliminary.  Even with the patch applied at least the following places in the code still call the global "operator new":

GCHashtable::GCHashtable
GCHashtable::grow
MMgc::GCStack::Alloc
MMgc::GC::AllocRCRoot
AvmCore::AvmCore (strings table, namespaces table)
List::ensureCapacity
AvmCore::rehashStrings
avmplus_pcre_malloc

Ditto, the 'delete' operators are called from several places.

I wish there was some way of making the compiler tell me where all these places are, but so far no luck (hints welcome...).  However, making the new and delete operators assert and then running the test suite, iteratively, should be enough to find most places.

Comment 23

[Lars T Hansen]

(In reply to comment #22)
> (In reply to comment #20)
> > Created an attachment (id=347538) [details] [details]
> > Patch to remove the MMgc global variables
> 
> This patch is incomplete and should be considered preliminary.

Tommy, thanks for the review+.  I will let this patch sit until after the release, as it is not release-critical.  That way I have time to track down uses of the new and delete operators.

Comment 24

[Lars T Hansen]

Nothing currently depends on this; archived for now.

Comment 25

[Lars T Hansen]

I believe this requirement finally fell off the table.

Comment 26

[Trevor Baker]

bulk verifying resolved !fixed issues