Closed Bug 612150 Opened 9 years ago Closed 9 years ago

Eliminating JS_GetFunctionName

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: igor, Assigned: igor)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

+++ 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.
Seems like jsd should be using the correct (not randomly ascii-ified) function name anyway, no?
Attached patch wip (obsolete) — Splinter Review
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.
Attached patch v1Splinter Review
The new version fixes couple of typos and comments and passes the try server.
Attachment #492661 - Attachment is obsolete: true
Attachment #493267 - Flags: review?(mrbkap)
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!
Attachment #493267 - Flags: review?(mrbkap) → review+
http://hg.mozilla.org/tracemonkey/rev/e35b70ffed69
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/e35b70ffed69
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
No longer blocks: 618549, 619641
Depends on: 618549, 619641
This has caused regressions: bug 618549 and bug 619641.
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
Depends on: 620510
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.
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
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.
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
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.
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
Depends on: 624880
You need to log in before you can comment on or make changes to this bug.