627246 - Remove "AvmObject" etc from nativegen and NativeFunction

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, enhancement)

Description

[Steven Johnson]

These typedef serve no purpose and just cloud our namespace with junk; we should use the real types directly, since the usages are machine generated.

Comment 1

[Steven Johnson]

Attachment: Patch

Comment 2

[Edwin Smith]

Comment on attachment 505268 [details] [diff] [review]
Patch

+1, this will just make life easier in a lot of ways.  I didn't
see anything of substance but here's what came to mind, reviewing
the patch.

> // bools are passed in as int32_t, but returned as bool, for historic reasons

Needs an explanation of what the historic reasons are, and caps/punctuation.  IIRC, this isn't the same issue as nanojit's lack of support for C++ 'bool' in native call ABI's.

> inline

time to normalize with REALLY_INLINE?  fine if its a different patch.

> AvmThunkBox_ScriptObject(r), etc

It looks like 1:1 correspondence with SlotStorageType, should we normalize names and capitalization?  i.e. SST_int32, AvmThunkUnbox_int32, etc.

> AvmAssert(sizeof(Atom) == sizeof(double));

This and others could be MMGC_STATIC_ASSERT

>  AvmThunkCoerce_Atom_double()

I know its nothing to do with this patch, but looking at these macros out
of context, I have *no* idea what they do.  the name implies coersion, but
all they do is return null or NaN.  Add Some comments?

> nativegen.py:744
> val = "AvmThunkGetConstantString("+str(i)+")/* \""+self.strings[i]+"\" */";

Nit, but python's printf-style formatting is easier to grok:

  val = 'AvmThunkGetConstantString(%s)/* "%s" */' % (i, self.strings[i])

Reading on in the patch, it looks like you did switch a few other
formatting expressions to be printf style (cool).  Oh, and I bet that one
would cause a C++ syntax error if the string constant contained */.
Not exactly a big deal for now.

I only skimmed the rest of the nativegen.py changes.  Testing is more likely
to catch bugs here than review.

Comment 3

[Steven Johnson]

pushed with minor changes as 5813:66b34035e6ce

Comment 4

[Brent Baker]

(In reply to comment #3)
> pushed with minor changes as 5813:66b34035e6ce

This patch is causing compilation errors on windows debugger builds (seen using x-plat build script)

core\builtin.cpp(970) : error C2220: warning treated as error - no 'object' file generated
core\builtin.cpp(970) : warning C4800: 'uint32_t' : forcing value to bool 'true' or 'false' (performance warning)
core\builtin.cpp(995) : warning C4800: 'uint32_t' : forcing value to bool 'true' or 'false' (performance warning)
core\builtin.cpp(1020) : warning C4800: 'uint32_t' : forcing value to bool 'true' or 'false' (performance warning)
core\builtin.cpp(1045) : warning C4800: 'uint32_t' : forcing value to bool 'true' or 'false' (performance warning)

Comment 5

[Brent Baker]

Attachment: workaround vs warning C4800

There are still issues with compiling windows debugger  builds that are not addressed:

core\avmpluslist-impl.h(367) : warning C4738: storing 32-bit float result in memory, possible loss of performance

core\bytearrayglue.cpp(710) : warning C4738: storing 32-bit float result in memory, possible loss of performance

core\dataio.cpp(255) : warning C4738: storing 32-bit float result in memory, possible loss of performance

Comment 6

[Edwin Smith]

Comment on attachment 505421 [details] [diff] [review]
workaround vs warning C4800

I can't tell where the bug is.

1. is the bug that the native getter for (e.g.) XML.ignoreComments() returns uint32_t instead of C++ bool?  if so, then the native getters/setters should be changed, and the warning goes away.

2. or, if native functions returning AS3 bool should return C++ uint32_t, then the bug is the generated code, and this patch looks okay.  (but maybe isn't enough, see the next comment from Brent)

Comment 7

[Edwin Smith]

(In reply to comment #5)

> There are still issues with compiling windows debugger  builds that are not
> addressed:
> 
> core\avmpluslist-impl.h(367) : warning C4738: storing 32-bit float result in
> memory, possible loss of performance
> 
> core\bytearrayglue.cpp(710) : warning C4738: storing 32-bit float result in
> memory, possible loss of performance
> 
> core\dataio.cpp(255) : warning C4738: storing 32-bit float result in memory,
> possible loss of performance

Its not at all clear whats causing these warnings.  were they injections from Steven's patch?

Comment 8

[Brent Baker]

(In reply to comment #7)
> (In reply to comment #5)
> 
> > There are still issues with compiling windows debugger  builds that are not
> > addressed:
> > 
> > core\avmpluslist-impl.h(367) : warning C4738: storing 32-bit float result in
> > memory, possible loss of performance
> > 
> > core\bytearrayglue.cpp(710) : warning C4738: storing 32-bit float result in
> > memory, possible loss of performance
> > 
> > core\dataio.cpp(255) : warning C4738: storing 32-bit float result in memory,
> > possible loss of performance
> 
> Its not at all clear whats causing these warnings.  were they injections from
> Steven's patch?

These errors occur because of the original patch. These errors were not visible in the build system, te appeared to have stopped at the first error, however if built in VS then the boolean warning (commment #4) and the 32-bit float error (comment #5) happen during a debugger compilation.

Comment 9

[Steven Johnson]

The bug is that the old thunks would cast the result more aggressively, so a native function returning uint32_t for a "bool" would get it explicitly cast. This cast is now removed; I'm going to fix the XML native functions to be declared as they should have been in the first place.

Comment 10

[Steven Johnson]

The 32-bit float warning is a bit baffling though; it's appearing in code that doesn't appear to be obviously related to my change... investigating

Comment 11

[Tamarin Bot]

changeset: 5816:bd2097c9f785
user:      Steven Johnson <[e-mail]>
summary:   Bug 627246 followup: fix cast warning for XMLClass on windows (r=stejohns)

http://hg.mozilla.org/tamarin-redux/rev/bd2097c9f785

Comment 12

[Steven Johnson]

I pushed a fix for the XMLClass casts... but the 'float' warnings predate my change, looking for injection point now

Comment 13

[Steven Johnson]

Comment on attachment 505421 [details] [diff] [review]
workaround vs warning C4800

rejecting because I chose to fix it in an alternate way, namely correcting the glue code to have correct declarations