487593 - Additional features of the configuration system

Status: VERIFIED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Lars T Hansen]

Tracking bug for additional ideas for the configuration system.

Comment 1

[Lars T Hansen]

Idea from Edwin channeled through me:

The configuration script should be able to generate code for configure.py to pick up and use, so that --enable-whatever becomes a command line switch of configure.py if AVM{FEATURE,SYSTEM}_WHATEVER is a known feature.

(For this to work well we also want our settings file, avmshell-features.h, to always #ifdef on the feature name before providing a default setting.  Minor issue.)

Comment 2

[Lars T Hansen]

Idea from Edwin channeled through me:

We have this pattern, for several features:

 - compiled out
 - compiled in, disabled
 - compiled in, enabled

Examples:  jit, debugger, cse-optimizations, cpu-feature-detection (eg
x87 v. sse2), self-tests

It'd be cool if we could use our feature system to also define the
config object we use for runtime switches, and maybe even to generate
the macros/functions for testing features in control-flow, so there'd be
no ifdefs in code:

  if (vmcfg_x87()) { ... } else { ... }

where vmcfg_x87 might be the constant 0, or an expression to check the
runtime config.  We see this pattern with the debugger and others too.

Brainstorming about that, presumably this would be a single parameter on the feature definition about whether it would be enablable or not,

  <feature>
   ...
   <run-time-switchable name="vmcfg_x87" default="false"/>
  </feature>

?  The default could be some expression and is used in the config object's constructor, the name turns into a setter/getter pair maybe (overloading prevents us from needing two names, inlining comes into play too).  It's a little harder to support mutually-exclusive choices (eg one or the other of the interpreters, say - not that i think that's really a use case) but since we're generating code it's not really all that difficult if it can be expressed declaratively.

Comment 3

[Lars T Hansen]

And of course an ongoing item: Moving more #ifdefs out of avmbuild and the various MMgc build files and into the feature system.

Comment 4

[Edwin Smith]

(In reply to comment #2)

> ?  The default could be some expression and is used in the config object's
> constructor, the name turns into a setter/getter pair maybe (overloading
> prevents us from needing two names, inlining comes into play too).  It's a
> little harder to support mutually-exclusive choices (eg one or the other of the
> interpreters, say - not that i think that's really a use case) but since we're
> generating code it's not really all that difficult if it can be expressed
> declaratively.

Yeah, should work.  Will require standardizing where the runtime configuration options actually live; some context object that transcends MMgc and AvmCore?

with every feature being a boolean this seems like a pretty tractable algebra.

Comment 5

[Lars T Hansen]

Attachment: Refactoring platform code, low hanging fruit

A little bit of a mishmash but two main things going on.  One, I've moved a fair amount of platform code from the core files out to the platform files.  This may leave Symbian builds hanging since it looks like they may have piggybacked on Unix, but I think this is OK.  Two, the #include of avmfeatures.h has moved from avmplus.h into VMPI.h in order to make clean names available in all the platform include files (eg, mac-platform.h).

(Includes bug fixes for SPARC compilation, some type names that migrated to the "_t" variant, mode lines.  Sorry.  Acceptable (if at all) only because the patch is not very large.)

Comment 6

[Edwin Smith]

Comment on attachment 372033 [details] [diff] [review]
Refactoring platform code, low hanging fruit

missing newline on VMPI.h:268, nothing else interesting.

Comment 7

[Lars T Hansen]

Attachment: Solaris compilation fixes relative to 372033

Comment 8

[Lars T Hansen]

Comment on attachment 372033 [details] [diff] [review]
Refactoring platform code, low hanging fruit

redux changeset:   1735:5efa6deb46da

Comment 9

[Lars T Hansen]

Comment on attachment 372053 [details] [diff] [review]
Solaris compilation fixes relative to 372033

redux changeset:   1736:59f315c22f8a

Comment 10

[Lars T Hansen]

Attachment: Indentation & white space in avmshell.cpp

Just trying to keep myself sane: this cleans up indentation by outdenting some #ifdefs and then reindenting according to that.  Also removes some unused cruft (PRIVATE for Solaris, not used in this file or indeed anywhere).

Comment 11

[Lars T Hansen]

Attachment: Generate avmfeatures.cpp and avmfeatures.py

More code generation:

Generates core/avmfeatures.cpp, which is all #ifdef AVMSHELL_BUILD and contains a string of the names of features compiled in to the code.  Will eventually be coupled with a command line switch to the shell to dump those names, and of course it will show up in the project files and manifest.mk.

Generates build/avmfeatures.py, which processes command line switches for standard names: for example, it recognizes --enable-selftest and transforms that into a definition of AVMFEATURE_SELFTEST=1.  It also handles exclusive settings, so --enable-abc-interp turns into AVMFEATURE_ABC_INTERP=1 AVMFEATURE_WORD_INTERP=0.  What I haven't figured out for this one yet is how to handle --disable-whatever for features that are on by default.

configure.py has been included to load avmfeatures.py and call the function defined therein.

Comment 12

[Lars T Hansen]

Comment on attachment 373091 [details] [diff] [review]
Indentation & white space in avmshell.cpp

redux changeset:   1760:5f4fcae4b9ce

Comment 13

[Lars T Hansen]

Should get rid of defined AVMPLUS_ARM_OLDABI and replace it with something based on AVMFEATURE_FP_WORDS_REVERSED, eg, VMCFG_FP_WORDS_REVERSED (each word is of the correct endianness but the two words are swapped).  If that's the right semantics; I believe they are.

There's a union that depends on AVMPLUS_ARM_OLDABI; it is defined three places, and that may not be necessary.

Comment 14

[Lars T Hansen]

Attachment: MMgc configuration reorg

This refactors and reorganizes the MMgc configuration lightly, making it depend on the already existing and debugged infrastructure in VMPI.h and the new feature system.  The files armbuild, winbuild, etc remain but contain very little now - only the stuff for the memory profiler, which I figure we can clean up later.

This patch has not been vetted by the buildbot yet but it's making its way through...

Comment 15

[Lars T Hansen]

Comment on attachment 373360 [details] [diff] [review]
MMgc configuration reorg

redux changeset:   1769:e937404a9c90

Comment 16

[Lars T Hansen]

Attachment: sundry cleanup work

This patch introduces feature and system definitions for previously hidden macros.  It removes a bunch of system file inclusions.  And it fixes some bugs I found in the process.  The patch is a little sprawling but it is virtually all refactoring I hope I can get away with it :-)

Not all macros are taken care of, and not all system file inclusions are eradicated.  See the next comment for more information about what work remains.

Comment 17

[Lars T Hansen]

Known remaining issues:

UNDER_CE remains an ad-hoc way of talking about Windows Mobile; WIN32 && ARM remains an ad-hoc way of talking about Windows Mobile.  We should probably have AVMPLUS_WINMO (which need not be exclusive with WIN32).

AVMPLUS_MACH_EXCEPTIONS - never defined, used twice in avmshellMac to set up and take down Mach exceptions.  Since it's shell code and never defined, we can probably remove it?

AVMPLUS_MAC_CARBON - never defined, used by the JIT for setjmp/longjmp compilation.  Do we still support Carbon APIs?

AVMPLUS_PORTING_API - this remains supported for the moment, because I don't want to just nuke the code in the JIT that uses it for icache flushing and page protection.  But it will have to go away, and those parts of the JIT need to be fixed.

Also MMGC_PORTING_API - used only for GetStackTop, and Tommy says this is on its way out.

The definition of PROT_TYPE and PROT_SIZE should be factored out of Domain.cpp and into platforms that enable page protection.  Part of cleaning up jit memory handling presumably.

Platform ifdefs in the code - how bad is it?  Not bad actually.  The ugliest remaining part has to do with page protection for the JIT, which is about to be rewritten anyway.  There are some architecture ifdefs throughout, and a lot of cases for x86 and x86-64 in math code, scattered throughout - not pretty.  (Remember both AVMPLUS_ARCH and MMGC_ARCH when grepping.)

System #include files?  JavaGlue.{h,cpp}, vprof.cpp, NativeARM.cpp, Assembler.cpp, GC.cpp, MathUtils.cpp, CodegenLIR.cpp, AvmCore.cpp.  Look easy to clean up, some may not be worth cleaning up at all.

There are some settings that are not cross-operating-system but cross-architecture, so it may be that we want to reflect that: now we have  unix-platform.h, mac-platform.h, etc; we may want to have ia32-platform.h, ppc-platform.h.  There's also the compiler dimension.  It's unclear
how well they would compose however so this remains speculative.

Comment 18

[Steven Johnson]

> AVMPLUS_MAC_CARBON - never defined, used by the JIT for 
> setjmp/longjmp compilation.  Do we still support Carbon APIs?

Not entirely clear. Let's check with Flash to ensure there are no corner cases remaining that might need support before we remove it.

Comment 19

[Lars T Hansen]

Two things I forgot to mention.

One is "raw" platform names eg "_MAC", "WIN32", "_WIN32", "LINUX", "UNIX", "_ARM_", "__ARM__", "__arm__", "__i386__", "__x86_64__", "__SSE2__".

Another is compiler names, eg "_MSC_VER", "__GCC__", "__GNUC__", "__MWERKS__".

I'm not proposing that all of these need change; some things just /are/ platform dependent and lifting them to a higher level just introduces complexity without benefit.  But most of these are probably used in checks that could either be moved into platform files or be redefined as features, and for the few that remain it seems like it would be useful to change the code so that it uses standard names that we control (ie, AVMPLUS_GCC or something like that) not the compiler's name for itself in some combination.

A favorite, which occurs twice in the code in two different files:

#if (defined(_MSC_VER) || (defined(__GNUC__) && defined(__SSE2__))) && (defined(AVMPLUS_IA32) || defined(AVMPLUS_AMD64))

Comment 20

[Lars T Hansen]

(In reply to comment #18)
> > AVMPLUS_MAC_CARBON - never defined, used by the JIT for 
> > setjmp/longjmp compilation.  Do we still support Carbon APIs?
> 
> Not entirely clear. Let's check with Flash to ensure there are no corner cases
> remaining that might need support before we remove it.

Tinic says we will continue to use Carbon for PPC builds, so the code stays at least until the Player drops PPC support.  I'll add a comment to record this fact.

Comment 21

[Lars T Hansen]

Another one: AVM_SHELL_NO_PROJECTOR

Comment 22

[Edwin Smith]

(In reply to comment #11)

> What I haven't figured out for this one yet is how
> to handle --disable-whatever for features that are on by default.

not sure if you've investigated.  Options.getBoolArg() takes a second optional parameter that is the default (true for args like this).

I a problem is that configure.py and avmshell-features.h need to be in league with each other since they both determine what the defaults are, whereas the main feature system doesn't have defaults.

Comment 23

[Edwin Smith]

(In reply to comment #17)

> AVMPLUS_MACH_EXCEPTIONS - never defined, used twice in avmshellMac to set up
> and take down Mach exceptions.  Since it's shell code and never defined, we can
> probably remove it?

Yes, I attempted to remove it entirely when MIR went away, I think
it reappeared due to the shell refactoring that happened around the
same time.  It enabled mach exception handlers required for GrowableBuffer

> System #include files?  JavaGlue.{h,cpp}, vprof.cpp, NativeARM.cpp,
> Assembler.cpp, GC.cpp, MathUtils.cpp, CodegenLIR.cpp, AvmCore.cpp.  Look easy
> to clean up, some may not be worth cleaning up at all.

Rick & I had a brief discussion about JavaGlue -- we should decide whether
to nuke it or support/test it, right now its essentially dead code.

Comment 24

[Steven Johnson]

I vote for nuking it, we can always resurrect it later if necessary

Comment 25

[Lars T Hansen]

Comment on attachment 373119 [details] [diff] [review]
Generate avmfeatures.cpp and avmfeatures.py

redux changeset:   1779:dc6f8adb41c1

Comment 26

[Lars T Hansen]

We need to fix the handling of VMCFG_METHODENV_IMPL32 and VMCFG_METHOD_NAMES are handled, they should all be #ifdef, not #if, and they should be exposed through the feature system.  But I don't want to mess with that until the patch in comment #16 has landed, as that performs a lot of cleanup and reorg already.

Comment 27

[Lars T Hansen]

Comment on attachment 373493 [details] [diff] [review]
sundry cleanup work

redux changesets 1783-1785

Comment 28

[Lars T Hansen]

-Dversion sould probably print a few things not covered by the feature system:

  DEBUG flag (printed as DEBUG or RELEASE ?)
  Compiler name & version, if known (printed as COMPILER_GCC, COMPILER_MSVC ?)

Comment 29

[Chris Dearman]

Attachment: Add support for --disable-feature

(In reply to comment #11)

> settings, so --enable-abc-interp turns into AVMFEATURE_ABC_INTERP=1
> AVMFEATURE_WORD_INTERP=0.  What I haven't figured out for this one yet is how
> to handle --disable-whatever for features that are on by default.

This patch allows "simple" features without dependencies to be explicitly disabled at configuration time. I've been using this to do builds with the JIT compiler disabled by specifying --disable-jit.

Comment 30

[Lars T Hansen]

Comment on attachment 396804 [details] [diff] [review]
Add support for --disable-feature

I think there's one bug that ought to be fixed here, which is that the generator code introduces names with mixed '-' and '_' separators when using '-' all the time seems more appropriate.  That bug is not introduced by this patch, though - it's caused by a using a non-global "replace" in the generator.

Comment 31

[Brent Baker]

Comment on attachment 396804 [details] [diff] [review]
Add support for --disable-feature

There is actually a problem here, the feature system does not know what the default/calculated values of all of these switches are. With this patch all features are specifically disabled unless they are explicitly set with the enable switch.

Comment 32

[Brent Baker]

Comment on attachment 396804 [details] [diff] [review]
Add support for --disable-feature

My comment (#31) was incorrect. I had another patch applied which modified the build/getops.py:getBoolArg() setting the default value to False instead of None.

Rebased the patch to redux tip and change the replace() in the avmfeatures.as to be a global replace. Pushed as changeset 2416:cf4fcbd12a7f

Comment 33

[Andre Kruetzfeldt]

Expired, revive it if you will.

Comment 34

[Trevor Baker]

bulk verifying resolved !fixed issues