467596 - Configuration system and platform abstraction

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Lars T Hansen]

The VM needs a credible configuration system, and a credible mechanism for platform abstraction.  We need to regularize feature definitions, record dependencies between them, provide a way for platforms to supply definitions for VMPI_ names, and so on.

Please record requirements / needs / ideas in comments.

Comment 1

[Edwin Smith]

see also 458393

Comment 3

[Edwin Smith]

added dependency on bug for adding a portable assert

Comment 4

[Rishit Shah]

Attachment: VMPI_ changes for std CRT functions and basic types [v1]

Comment 5

[Rishit Shah]

Attachment: New file containing includes and defines on per-platform basis [v1]

Comment 6

[Rishit Shah]

NEW FILES:
<root>/platform/VMPI.h

TEST INFO:
Passes acceptance test on Mac OS X

PROJECT FILES TO UPDATE:
Projects for all existing platforms

ATTACHMENTS
Patch for changeset
VMPI.h (to be added)

NOTE:
Following files no longer required (could/should be removed from source tree)
mmgc_stdint.h 
avmplus_stdint.h

Comment 7

[Lars T Hansen]

Comment on attachment 356794 [details]
New file containing includes and defines on per-platform basis [v1]

Needs to be broken into individual files for each platform.

Comment 8

[Lars T Hansen]

As a general comment on patch #2, we should avoid using #elif for the platforms, because it makes maintaining variants harder than it needs to be.  A simple #ifdef .. #endif pair for each platform is better, with a flatter file structure.  (And then a single #include line for each platform IMO.)

Comment 9

[Lars T Hansen]

Comment on attachment 356792 [details] [diff] [review]
VMPI_ changes for std CRT functions and basic types [v1]

So far as I can tell this patch does three separate things.  It fixes some option names useed in the GC; it cleans up the header file mess; and it renames calls to the libc functions by prefixing them with VMPI_.  I think these should be delivered as three patches (presumably in that order) that can be reviewed individually, both for general cleanliness and because I suspect the third patch is not right yet.  I noticed some stdio functions (fgets? fputs?) that were not converted like the others were, and it's an open question whether platform-specific code should be converted at all.

Comment 10

[Reed Loden [:reed]]

The content of attachment 356794 [details] has been deleted by
    Reed Loden [:reed] <[e-mail]>
who provided the following reason:

Requested by creator for reasons specified in bug 473504.

The token used to delete this attachment was generated at 2009-01-13 23:08:38 PST.

Comment 11

[Lars T Hansen]

Attachment: Draft configuration system

This draft works in current tamarin-redux.  It has the following components.

The script core/avmplusFeatures.as contains the set of system features as an XML datum, and when the script is run it prints a header file on stdout; the output should be captured as core/avmplusFeatures.h.  (Bug to fix: it should just write the output to that file.)

The file avmplusFeatures.h checks feature dependencies and computes internal names controlling the code.  Anything we can think of in terms of static checks should be incorporated; nobody cares if this file is big as long as it is comprehensible.

VMPI.h includes the product configuration file that provides values for all known features.  For the avmshell this file is shell/features.h, and VMPI.h fetches it if AVMPLUS_SHELL is defined.  Otherwise, VMPI.h fetches ../../AVMPlusConfig/features.h, ie, some file defined by the embedder, outside the tamarin code base.

avmplus.h includes core/avmplusFeatures.h before it includes avmbuild.h; avmplusFeatures.h + features.h is meant to take over most of the work of avmbuild.h.

Much of avmbuild.h has therefore been moved into shell/features.h, selecting platform settings in the same way it used to, more or less.  The rest of avmbuild.h remains where it was and is effectively a post-pass on the feature configuration; the long-term goal is to reduce this to a very small amount of code.  (Some will invariably remain.)

I have defined a bunch of features, into two groups.  AVMPLUS_SYSTEM_* are system settings: architecture, word length, endianness, operating system.  AVMPLUS_FEATURE_* controls product settings: whether the JIT is on or not, which interpreter to use, whether UTF32 strings are on, and so on.  If you ever wonder about the size of the cross-product of feature settings - this is where you find the canonical list of features.

There are probably plenty of features in the code that I have not yet identified as such, so the set of feature defs will grow.

There is also a set of settings I've not tackled yet, namely platform capabilities.  For example, does the platform support mmap (really, VMPI_mmap)?  I think this is a bigger discussion that Rishit will probably prefer to keep the control over, given that he's working on the porting layer.

I received some comments on an earlier draft, here are some replies.

- Operating systems are simply AVMPLUS_SYSTEM_UNIX, etc.  These never imply
  anything about the CPU architecture, and there may be other variations that
  must be 

- I have not introduced an alias facility to allow old-style names to translate
  into the new feature definitions, because doing so is antithetical to the
  idea of the feature system and asynchronous integration.

Comment 12

[Steven Johnson]

super-nit here, but at this point perhaps we should consider moving away from "avmplus" as a name component. It made sense at one time, but we seem to be gravitating towards avm, AVM, Avm, etc as our qualifying prefix.

Comment 13

[Rob Borcic]

Aren't operating systems supposed to be abstracted out by the VMPI porting layer? There shouldn't be any OS specific ifdefs anywhere except in the porting layer redirector VMPI.h.

Comment 14

[Lars T Hansen]

(In reply to comment #13)
> Aren't operating systems supposed to be abstracted out by the VMPI porting
> layer? There shouldn't be any OS specific ifdefs anywhere except in the porting
> layer redirector VMPI.h.

In the best of all worlds, yes, and maybe even in our own imperfect one.  But we don't have the time to fix all the problems during this sprint.  I'm guessing OS switches in the code will be with us for months to come, and fixed on demand; my only requirement is that their number be nonincreasing at every check-in.

(I'm happy to be proved wrong.)

Comment 15

[Lars T Hansen]

(In reply to comment #12)
> super-nit here, but at this point perhaps we should consider moving away from
> "avmplus" as a name component. It made sense at one time, but we seem to be
> gravitating towards avm, AVM, Avm, etc as our qualifying prefix.

I agree, and if you're sure we won't be conflicting with (ahem) embedders' use of the unqualified prefix "AVM_" in other contexts we could try to push for that.  It would give us four more letters of useful payload in names (preprocessors /do/ limit names to 31 characters).

Comment 16

[Rishit Shah]

What is the placeholder for all the VMPI API declarations?  Adding them to VMPI.h might clutter it.

Comment 17

[Lars T Hansen]

(In reply to comment #16)
> What is the placeholder for all the VMPI API declarations?  Adding them to
> VMPI.h might clutter it.

I'm not sure I understand this comment, could you elaborate?

Comment 18

[Rishit Shah]

We'll be adding declarations for porting APIs.  

For example, VMPI_Log, VMPI_alloc, etc.

I was wondering whether we add those declarations to VMPI.h or should that be a separate header?

Comment 19

[Lars T Hansen]

Separate header probably; we supply definitions for tier one platforms at least and leave the other platforms up to porting teams.  But I think this/these headers should be included from VMPI.h, in case that wasn't clear.

Comment 20

[Lars T Hansen]

(In reply to comment #19)
> Separate header probably; we supply definitions for tier one platforms at least
> and leave the other platforms up to porting teams.  But I think this/these
> headers should be included from VMPI.h, in case that wasn't clear.

That is, at least indirectly from VMPI.h...

Comment 21

[Lars T Hansen]

(In reply to comment #12)
> super-nit here, but at this point perhaps we should consider moving away from
> "avmplus" as a name component. It made sense at one time, but we seem to be
> gravitating towards avm, AVM, Avm, etc as our qualifying prefix.

I think "AVMFEATURE_" is a good prefix for definitions controlling functionality, and "AVMSYSTEM_" is a good prefix for definitions controlling platform aspects (CPU, word size, endianness, etc).

Comment 22

[Lars T Hansen]

Attachment: Draft configuration system, v3

This is clean, compiles on mac, windows, xplat.  Significant refactoring in the platform directory now, with VMPI.h cut to the bone and all platform-dependent files moved into the platform subdirectories, whence they are included.  The feature configuration file is included from some directory in the path; different file for the shell and a host embedding.  (So for the player there's a little work to do here for integration.)  Our project files all define AVMPLUS_SHELL to trigger inclusion of the shell feature file.

Also refactored is platform determination (OS, CPU, endianness, word size); it has been removed from avmbuild.h into platform/system-selection.h; it is used by the shell feature file and can be used as-is by any other feature file.

avmbuild.h has been stripped a fair amount, but not everything that needs to be a feature is a feature at this point, we can go slow.

it would be good to have reviews this week, do one more round of refinement, and try to land it after the next TR -> TC merge, unless that drags on well into the next sprint.

Comment 23

[Steven Johnson]

Comment on attachment 366672 [details] [diff] [review]
Draft configuration system, v3

-- I am a bit troubled by the idea that VMPI.h includes different things depending on whether we build for shell or not... this feels like a recipe for subtle integration errors.

-- nit: we've avoided using relative include paths (eg "../platform/system-selection.h" in the past. is there a reason we can't continue this practice?

-- nit: "avmplusFeatures.h", I thought we were avoiding the "plus" for new files?

Comment 24

[Lars T Hansen]

The ../platform/system-selection.h is a forgotten edit.  WILLFIX.

The avmplusFeatures.h is an oversight.  WILLFIX.

But I don't understand how you can be troubled by the idea that VMPI.h includes different things depending on whether we build for shell or not.  That it does so is a fundamental idea underlying the configuration system - different products (embeddings of avm) have different configurations.  Big machines have more features than small machines, for example.  So the file to be included (containing feature definitions) is specific to the embedder.  Since we are our own embedder for the shell, a small concession to this fact was made in VMPI.h so that we can avoid having a file in our source tree that the embedding host software will also need to have in its source tree (avmhost-features.h).  We could solve this by having two source repositories, one for the shell and one for the rest of the vm, and check the latter out as a subdirectory of the other to mimic what goes on in other embeddings, and then the inclusion in VMPI.h could be simpler (it would always include avmhost-features.h, and the shell repository would provide one).

Comment 25

[Steven Johnson]

(In reply to comment #24)
> We
> could solve this by having two source repositories, one for the shell and one
> for the rest of the vm, and check the latter out as a subdirectory of the other
> to mimic what goes on in other embeddings, and then the inclusion in VMPI.h
> could be simpler (it would always include avmhost-features.h, and the shell
> repository would provide one).

That does sound cleaner, but probably not worth doing unless the extra work required is trivial. I'm happy enough with your proposed solution if you think it's the right compromise.

Comment 26

[Lars T Hansen]

Rishit observes that AVMPLUS_SHELL is an old-style name and that we may consider something else.  (I agree and favor perhaps something like AVMSHELL_BUILD, to make it explicit and unlikely to clash.)  May or may not happen for the initial release; depends on timing.

Comment 27

[Lars T Hansen]

I have corrected the .. include paths, the name of the include file, and the preprocessor name for shell builds, but otherwise nothing.  If anyone wants a last look and opportunity to say "no" then speak now, otherwise this lands tomorrow morning CET.

Comment 28

[Lars T Hansen]

redux changeset:   1716:8bce148353ab