474304 - project files define AVMPLUS_IA32 on x86-64 and arm configs

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Edwin Smith]

Incorrectly, of course.

Since we can and do detect cpu in headers from compiler-defined flags, we can remove AVMPLUS_CPU flags entirely from the project files.

This patch does that.

Comment 1

[Edwin Smith]

Attachment: remove AVMPLUS_ARM, _IA32, _AMD64 from build/project files

Comment 2

[Edwin Smith]

Attachment: rebased, fixed unixcpuid.cpp to include avmplus.h

Comment 3

[Edwin Smith]

Comment on attachment 359337 [details] [diff] [review]
rebased, fixed unixcpuid.cpp to include avmplus.h

Rick: this adds errors for defining IA32 and AMD64 at the same time

Lars: if this chaffes the new portability reality, lmk.  its better than before i think.

Comment 4

[Rick Reitmaier]

Comment on attachment 359337 [details] [diff] [review]
rebased, fixed unixcpuid.cpp to include avmplus.h

in avmbuild.h do we really need to check for both ia32 && amd64 being set?  Although historically it occurred, we don't do it for other arch. (e.g. ia32 && ppc )

Not part of this patch but just pointing it out since you've adjusted the defines; should we stndize on NANOJIT_ and change an NJ_ defs?

Comment 5

[Lars T Hansen]

Comment on attachment 359337 [details] [diff] [review]
rebased, fixed unixcpuid.cpp to include avmplus.h

If it's better than before then that's all that counts.

Comment 6

[Edwin Smith]

(In reply to comment #4)

> (From update of attachment 359337 [details] [diff] [review])
> in avmbuild.h do we really need to check for both ia32 && amd64 being set? 
> Although historically it occurred, we don't do it for other arch. (e.g. ia32 &&
> ppc )

It's safer this way in the face of code that looks like this:

#if AVMPLUS_IA32 
  ...
#elif AVMPLUS_AMD64 
  ...
#endif

Which fails in weird ways if both are defined.  i didn't add checks for other
cpu's since it hasn't been a problem.

> Not part of this patch but just pointing it out since you've adjusted the
> defines; should we stndize on NANOJIT_ and change an NJ_ defs?

as you say, not part of this patch. just removing AVMPLUS where a NJ/NANOJIT
define exists instead.  if anything i'd prefer to have no redundant nanojit
flags, and/or standardize on NJ since its shorter.

Comment 7

[Lars T Hansen]

(In reply to comment #6)
> as you say, not part of this patch. just removing AVMPLUS where a NJ/NANOJIT
> define exists instead.  if anything i'd prefer to have no redundant nanojit
> flags, and/or standardize on NJ since its shorter.

Might I propose that you follow your own suggestion and start using VMCFG_ as the prefix for all internal configuration names (in anticipation of AVMPLUS_FEATURE_ being the external name)?  I've started doing it.  I like it; clarity improves.

Comment 8

[Edwin Smith]

This patch didn't introduce new names, so in the interest of doing one thing per patch, i'll create a second patch that does the global renaming.  in the interest of not boiling the ocean, how about i limit to the cpu flags.

what did we decide about the mmgc/nanojit/avmplus redundance.  best to rename all to use one set of flags?  ie VMCFG_64BIT replaces MMGC_64BIT, AVMPLUS_64BIT, and NANOJIT_64BIT?

Comment 9

[Steven Johnson]

> what did we decide about the mmgc/nanojit/avmplus redundance.  best to rename
> all to use one set of flags?  ie VMCFG_64BIT replaces MMGC_64BIT,
> AVMPLUS_64BIT, and NANOJIT_64BIT?

+1

Comment 10

[Lars T Hansen]

(In reply to comment #8)
> This patch didn't introduce new names, so in the interest of doing one thing
> per patch, i'll create a second patch that does the global renaming.

Cool.

> in the
> interest of not boiling the ocean, how about i limit to the cpu flags.

Ditto.

> what did we decide about the mmgc/nanojit/avmplus redundance.  best to rename
> all to use one set of flags?  ie VMCFG_64BIT replaces MMGC_64BIT,
> AVMPLUS_64BIT, and NANOJIT_64BIT?

IMO yes.

Comment 11

[Edwin Smith]

Comment on attachment 359337 [details] [diff] [review]
rebased, fixed unixcpuid.cpp to include avmplus.h

pushed to tamarin-redux

Comment 12

[Edwin Smith]

changeset:   1367:7886c215ac28

Comment 13

[Chris Peyer]

Resolved fixed engineering / work item that has been pushed.  Setting status to verified.