612150 - Eliminating JS_GetFunctionName

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

+++ This bug was initially created as a clone of Bug #607292 +++

With the patches for the bug 607292 and the bugs it depends on the only user of the deflated string is JS_GetFunctionName(JSFunction *) the API which is only used in jsd.

It would be nice to eliminate that and simplify the GC while reducing a source of potential GC hazards.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Seems like jsd should be using the correct (not randomly ascii-ified) function name anyway, no?

Comment 2

[Igor Bukanov]

Attachment: wip

Here is something that compiles. The patch does not changes XPCOM interfaces as they use nsACString strings and not the C strings. So the changes here only affects the direct users of JS_GetFunctionName and js/jsd/jsdebug.h API.

Comment 3

[Igor Bukanov]

Attachment: v1

The new version fixes couple of typos and comments and passes the try server.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 493267 [details] [diff] [review]
v1

>diff --git a/js/jsd/jsd_scpt.c b/js/jsd/jsd_scpt.c
> static void
> _dumpJSDScript(JSDContext* jsdc, JSDScript* jsdscript, const char* leadingtext)
> {
>     const char* name;
>-    const char* fun;
>+    JSString* fun;
>     uintN base;
>     uintN extent;
>     char Buf[256];
>     
>     name   = jsd_GetScriptFilename(jsdc, jsdscript);
>     fun    = jsd_GetScriptFunctionName(jsdc, jsdscript);
>     base   = jsd_GetScriptBaseLineNumber(jsdc, jsdscript);
>     extent = jsd_GetScriptLineExtent(jsdc, jsdscript);
>     
>-    sprintf( Buf, "%sscript=%08X, %s, %s, %d-%d\n", 
>-             leadingtext,
>-             (unsigned) jsdscript->script,
>-             name ? name : "no URL", 
>-             fun  ? fun  : "no fun", 
>-             base, base + extent - 1 );
>+    size_t n = size_t(snprintf(Buf, sizeof(Buf), "%sscript=%08X, %s, ",   

Doesn't the declaration of |n| need to be above the assignments just above this? (also, there's some trailing whitespace on this line).

It's nice to see all the code removal at the end of this patch!

Comment 5

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/e35b70ffed69

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e35b70ffed69

Comment 7

[David Mandelin [:dmandelin]]

This has caused regressions: bug 618549 and bug 619641.

Comment 8

[Andrew Paprocki]

In the 2.0b8 branch, I see a broken jsd_obj.c with this change..

    const char* ctorName;
...
                ctorName = jsd_GetScriptFunctionName(jsdc, jsdscript);

This returns a JSString* now. Getting:

jsd_obj.c: In function 'jsd_Constructing':
jsd_obj.c:160: error: assignment from incompatible pointer type

Comment 9

[timeless]

So, technically the changes done here are absolutely broken.

http://hg.mozilla.org/mozilla-central/diff/e35b70ffed69/js/jsd/jsdebug.c

    1.12 -JSD_PUBLIC_API(const char*)
    1.13 +JSD_PUBLIC_API(JSString *)
    1.14  JSD_GetScriptFunctionName(JSDContext* jsdc, JSDScript *jsdscript)

The definition of PUBLIC is "this case be used by code outside the module and thus will not change" the PUBLIC is not a promise on the argument, it's a decoration on the function, here JSD_GetScriptFunctionName.

With my module owner hat, I cry fowl against the reviewer for violating the sanctity of my module.

If you want to change the API, then you need to give the function a new name. There's nothing wrong with giving it a new name.

I don't have time to find current references to module policies, but I'll cite a dated one (and then I'll fly off to a wedding - during which time you're free to remove my module owner title).

http://www.mozilla.org/hacking/reviewers.html

> What needs super-review?
> Any change to any API or pseudo-API
> APIs are not just XPCOM APIs, but include global JS utility functions and the like.
> Designing these better up front makes it easier to build things on top of these APIs,
> and helps us avoid compatibility-killing "API cleanup" down the road.

Note that jsd is not part of JavaScript in the sense that it gets a "free pass", it's an independent module which happens to be related to javascript, it's ok to make changes in jsd on the tracemonkey branch in order to make a commit to tracemonkey, but it isn't ok to violate API rules.

Here are the rules for jsd:
1. If a function is declared as *_PUBLIC_API() then its arguments and return type and behavior must not change. If you need to change it then you need to create a new function (this can be a rename).
2. Ideally you'd leave a stub for the old function
3. If you feel this is impossible then you should either remove its implementation (leaving the definition and a comment indicating when it was removed)  or replace its implementation with a stub which aborts -- you may choose between aborting only ifdef DEBUG or unconditionally aborting -- I do provide some freedom.

As a handy hint, jsd like js uses JSD_ (like JS_) to indicate a PUBLIC_API. Items which are of the form jsd_ are private and can thus be changed at will.

Here are the rules for jsd_xpc:
1. if you change a property or method or the behavior of a property or method of an interface in an idl file, you must bump the iid for that interface.
2. When you change the iid of a type, you must change the iid's of all interfaces which use that type. Yes this means that on average if you change one fundamental iid in jsd you need to change nearly all of them, that's ok, iid's are cheap.

Comment 10

[Brendan Eich [:brendan]]

Timeless: we broke the API. We were never going to keep the old name and return type or other parts of the signature. There's no way to stub the old function so that it doesn't break its users.

Fast compile-time breakage is better than leaving a stub and a consequent runtime bug that may or may not be noticed.

When your upstream breaks compatibility in a way that passes through your module's APIs in a direct sense (like, you were veneering JS API and building on the ownership and char type models we broke), then the changes need to go in to keep the trees (not just tracemonkey) from burning.

You the downstream module owner should be involved, that was a mistake. Sorry for not catching it. But you weren't going to stop the change in any event.

What IID needed to change?

/be

Comment 11

[timeless]

brendan: this is C, return type changes have no influence on anything (SpiderMonkey is C++, JSD is not). So no, you didn't trip on a compile failure. nor did you trip on a linker failure.

$ cat a.c b.c; gcc -c a.c; gcc -c b.c; gcc a.o b.o -o test; ./test
/* a.c */
const char * x(void) {
return "hello world\n";
} 
/* b.c */
#include <stdio.h>
const int * x();

int main(void) {
  printf("%d", *x());
  printf("%s", x()); /* this line is here just so that I can get a newline in the output, it wouldn't be there in a real implementation */
/* the more amusing version of course is changing from const char * to const int * with the caller indexing the array, but I don't feel like crashing, just catching a plane */
}
b.c: In function ‘main’:
b.c:6: warning: format ‘%s’ expects type ‘char *’, but argument 2 has type ‘int *’
1819043176hello world

I'm not asking you to do something interesting on the JS side of things, i'm asking you not to do things wrong on the JSD side of things, that doesn't cost much, it does require not using the same function name.

The IID statement is because I felt I should list my rules in one place.

Comment 12

[Brendan Eich [:brendan]]

Timeless: was the only use in a printf? I don't think so, but if so: touché. C and C++ have no type safety there.

Nevertheless, we weren't going to stub and buy a runtime bug when we could break downstreams at compile-time. Hear me on this, it's not going to change.

Repeating your rules expends goodwill you want in return for that which you rightly expect from upstreams. No need to lecture summarily, let's focus on the exact problem or problems here. I agree there is at least one (lack of r?you).

/be

Comment 13

[timeless]

brendan: your changes to JS are fine. But the changes to JSD were not. All someone had to do was break things at compile time (of any JSD consumers) by renaming the JSD_ function.

Note that I'm unavailable for the next two weeks. I have a flight to catch in <8 hours.

Comment 14

[Brendan Eich [:brendan]]

Timeless: I will have to fill in for you. Speak fast if you disagree with any of my filling in here:

Igor, could you please file a bug to rename JSD_GetFunctionName to (this mirrors the JS API) JSD_GetFunctionId? We should do this per timeless's point about C not checking, so downstreams getting at most warnings.

/be