Last Comment Bug 578478 - xpidl.py can't parse [deprecated] attributes/methods
: xpidl.py can't parse [deprecated] attributes/methods
Status: RESOLVED FIXED
fixed-in-bs
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
: 584960 (view as bug list)
Depends on: 677395 676231
Blocks: 584971 584974 584997 584998 407216 578790 584967 584970 584976 584977 584981 584982 584983 585015 660861
  Show dependency treegraph
 
Reported: 2010-07-13 14:00 PDT by David Humphrey (:humph)
Modified: 2011-08-12 01:03 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
IDL Test Case with [deprecated] member (163 bytes, application/octet-stream)
2010-07-13 14:00 PDT, David Humphrey (:humph)
no flags Details
IDL Test Case with [deprecated] member (163 bytes, text/plain)
2010-07-13 14:01 PDT, David Humphrey (:humph)
no flags Details
PyXPIDL header changes, rev. 1 (12.64 KB, patch)
2010-07-14 12:53 PDT, Benjamin Smedberg [:bsmedberg]
jorendorff: review+
mbeltzner: approval2.0-
Details | Diff | Review
Create deps from the IDL parser, rev. 1 (8.40 KB, patch)
2010-07-29 14:44 PDT, Benjamin Smedberg [:bsmedberg]
khuey: review+
Details | Diff | Review
Fix race condition and post-fatvals changes (109.00 KB, patch)
2010-09-26 09:24 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Review
Final fixups (66.22 KB, patch)
2010-10-25 10:13 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
benjamin: review+
Details | Diff | Review
Modified version for checking (18.87 KB, patch)
2011-06-27 18:28 PDT, Joshua Cranmer [:jcranmer]
no flags Details | Diff | Review
Synchronize xpidl.py with native xpidl (14.32 KB, patch)
2011-07-18 14:23 PDT, Joshua Cranmer [:jcranmer]
no flags Details | Diff | Review

Description David Humphrey (:humph) 2010-07-13 14:00:02 PDT
Created attachment 457143 [details]
IDL Test Case with [deprecated] member

Currently xpidl.py fails when parsing IDL members flagged [deprecated]:

Traceback (most recent call last):
  File "header.py", line 443, in <module>
    idl = p.parse(open(file).read(), filename=file)
  File "/home/dave/moz/mozilla-central/xpcom/idl-parser/xpidl.py", line 1321, in parse
    return self.parser.parse(lexer=self)
  File "/usr/lib/python2.6/site-packages/ply/yacc.py", line 265, in parse
    return self.parseopt_notrack(input,lexer,debug,tracking,tokenfunc)
  File "/usr/lib/python2.6/site-packages/ply/yacc.py", line 971, in parseopt_notrack
    p.callable(pslice)
  File "/home/dave/moz/mozilla-central/xpcom/idl-parser/xpidl.py", line 1231, in p_member_method
    raises=p[7])
  File "/home/dave/moz/mozilla-central/xpcom/idl-parser/xpidl.py", line 765, in __init__
    raise IDLError("Unexpected attribute '%s'", aloc)
xpidl.IDLError: error: Unexpected attribute '%s', /tmp/deprecated-test.idl line 4:3
  [deprecated] boolean bad();
   ^
We have a few places in the tree where we do this now:

other-licenses/ia2/Accessible2.idl
storage/public/mozIStorageStatementWrapper.idl
storage/public/mozIStorageBaseStatement.idl
storage/public/mozIStorageStatement.idl

I've attached a small test case.
Comment 1 David Humphrey (:humph) 2010-07-13 14:01:22 PDT
Created attachment 457144 [details]
IDL Test Case with [deprecated] member
Comment 2 Benjamin Smedberg [:bsmedberg] 2010-07-14 12:53:11 PDT
Created attachment 457367 [details] [diff] [review]
PyXPIDL header changes, rev. 1
Comment 3 David Humphrey (:humph) 2010-07-14 13:51:35 PDT
Tested this against trunk (Linux), works great!
Comment 4 Jason Orendorff [:jorendorff] 2010-07-21 16:02:31 PDT
Comment on attachment 457367 [details] [diff] [review]
PyXPIDL header changes, rev. 1

Turning on pyxpidl is a bold change. Sounds good to me though.
Comment 5 Benjamin Smedberg [:bsmedberg] 2010-07-29 14:44:15 PDT
Created attachment 461347 [details] [diff] [review]
Create deps from the IDL parser, rev. 1
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-08-02 16:49:32 PDT
Comment on attachment 461347 [details] [diff] [review]
Create deps from the IDL parser, rev. 1

Shame optparse doesn't give you a way to enforce "these two options must come together" on its own.
Comment 7 Benjamin Smedberg [:bsmedberg] 2010-08-04 13:12:17 PDT
Comment on attachment 457367 [details] [diff] [review]
PyXPIDL header changes, rev. 1

I'm going to take these now, so DXR can be happier.
Comment 8 Benjamin Smedberg [:bsmedberg] 2010-08-06 07:49:43 PDT
*** Bug 584960 has been marked as a duplicate of this bug. ***
Comment 9 Walter Meinl 2010-08-09 14:33:00 PDT
shouldn't this raise the need for python > 2.4? I just got a build error with .rpartition not contained in str when building with python-2.4.4. Googling around I found that .rpartition was introduced with python-2.5.
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-09-25 19:17:14 PDT
Did these land?
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-09-25 19:18:31 PDT
Hg says yes.

http://hg.mozilla.org/mozilla-central/rev/81344fe5f7a0

Marking FIXED.
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-09-25 19:20:53 PDT
Ah, this got backed out.
Comment 13 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-09-26 09:24:47 PDT
Created attachment 478652 [details] [diff] [review]
Fix race condition and post-fatvals changes

To fix the race condition we just check in the generated files.

The fatvals stuff is slightly more interesting.  Right now "jsval foo(in jsval bar)" has the signature "NS_IMETHOD foo(const jsval& bar, jsval* foo)" which I think is a bug.  This changes it to be references across the board and fixes up the tree.
Comment 14 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-09-26 12:34:38 PDT
That patch causes some of the indexeddb tests to fail.
Comment 15 Benjamin Smedberg [:bsmedberg] 2010-10-04 11:37:08 PDT
khuey, are you sure that "references across the board" is correct? jsval* foo sound like the correct outparam form of a jsval return value.

Of course, matching the existing behavior of binary-xpidl is probably more important.

Checking in the generated files won't actually work unless you also somehow teach ply not to attempt to regenerate them if the timestamps don't work. And it seems really icky to me anyway... thoughts?
Comment 16 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-10-25 09:28:00 PDT
(In reply to comment #15)
> khuey, are you sure that "references across the board" is correct? jsval* foo
> sound like the correct outparam form of a jsval return value.

Why?  I think the code at http://mxr.mozilla.org/mozilla-central/source/xpcom/typelib/xpidl/xpidl_typelib.c#877 is bogus.  In particular, nsrootidl defines jsval as 

"[ref, jsval]  native jsval(jsval);"

The code in the binary xpidl ignores the ref modifier, which is a bug.

> Of course, matching the existing behavior of binary-xpidl is probably more
> important.

Except that it's wrong.  I think we either need to change the definition of jsval or change the xpidl compiler for 2.0.  Noming for blocking because of that.
 
> Checking in the generated files won't actually work unless you also somehow
> teach ply not to attempt to regenerate them if the timestamps don't work. And
> it seems really icky to me anyway... thoughts?

AFAICT ply doesn't rewrite the files to disk if it reads them in successfully, so I don't think that's an issue.
Comment 17 Benjamin Smedberg [:bsmedberg] 2010-10-25 09:33:04 PDT
I believe it deletes/rewrites them if the modification time of any of the input files is later than the cache file.

jsval is a [ref] inparam so that we know the size (pointer-size), since jsval is a 64-bit int or struct size depending on platform. I'll assert that the correct types for jsval are:

(const jsval& inParam, jsval* outParam);

Although I'd also accept:

(const jsval& inParam, jsval&* outParam);

Either way, for now we should just do whatever the binary xpidl does. jsval is a special type anyway.
Comment 18 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-10-25 09:35:07 PDT
(In reply to comment #17)
> I believe it deletes/rewrites them if the modification time of any of the input
> files is later than the cache file.

Right, so that just means that if the input files change we have to check in the updated generated files too, right?

> jsval is a [ref] inparam so that we know the size (pointer-size), since jsval
> is a 64-bit int or struct size depending on platform. I'll assert that the
> correct types for jsval are:
> 
> (const jsval& inParam, jsval* outParam);
> 
> Although I'd also accept:
> 
> (const jsval& inParam, jsval&* outParam);
> 
> Either way, for now we should just do whatever the binary xpidl does. jsval is
> a special type anyway.

OK.
Comment 19 Benjamin Smedberg [:bsmedberg] 2010-10-25 09:48:07 PDT
hg doesn't preserve file-modification times when you clone, so if the cache files are written by hg before xpidl.py, the very first build at least would cause PLY to remake the cache files.
Comment 20 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-10-25 10:13:28 PDT
Created attachment 485761 [details] [diff] [review]
Final fixups

Modify xpidl.py to handle jsvals the same way binary xpidl does and move the cache dir to the tree.  Also check in the generated files.

I've verified that if I touch xpidl.py and reexport the tree ply doesn't regenerate the generated files.  From poking around in ply's source, it looks like passing optimize=1 (which we do) shortcuts any "check if the generated files are valid" logic and uses them without question.
Comment 21 Mike Beltzner [:beltzner, not reading bugmail] 2011-02-23 10:41:49 PST
Comment on attachment 457367 [details] [diff] [review]
PyXPIDL header changes, rev. 1

I don't think we'd let this into the tree anymore; renominate if you disagree, and explain why!
Comment 22 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-04-10 21:44:35 PDT
I've updated these patches, but they totally destroy our buildtime on Windows (25% or more in my test).
Comment 23 Ted Mielczarek [:ted.mielczarek] 2011-04-11 05:55:13 PDT
We should patch bug 585015 and test with it as a pymake native command.
Comment 24 Joshua Cranmer [:jcranmer] 2011-06-27 18:28:28 PDT
Created attachment 542353 [details] [diff] [review]
Modified version for checking

This appears to compile everybody, at least by running with this as my myrules.mk:
ifneq ($(XPIDLSRCS),)
XPIDL_DEPS = \
             $(topsrcdir)/xpcom/idl-parser/header.py \
             $(topsrcdir)/xpcom/idl-parser/xpidl.py \
             $(NULL)
$(XPIDL_GEN_DIR)/%-py.h: %.idl $(XPIDL_GEN_DIR)/%.h $(XPIDL_DEPS)
  $(PYTHON) $(topsrcdir)/config/pythonpath.py \
    -I$(topsrcdir)/other-licenses/ply \
    -I$(topsrcdir)/xpcom/idl-parser \
    $(topsrcdir)/xpcom/idl-parser/header.py --cachedir=$(DEPTH)/xpcom/idl-parser -I $(DIST)/idl $(_VPATH_SRCS) > $@

check-%-idl: $(XPIDL_GEN_DIR)/%-py.h $(XPIDL_GEN_DIR)/%.h
  diff -Bw -I'/\*.*\*/' $^
export:: $(patsubst %.idl,check-%-idl, $(XPIDLSRCS))
endif

Note that I disabled document comments in xpidl_header (there's one document comment somewhere which is stashed at the end of the interface... trying to keep track of that just isn't worth it). I also ignored the fact that the attributes just constantly get wrongly ordered, and ignored the various whitespace changes. I'll see if I can get a profile to see where time improvements could be made.
Comment 25 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-07-17 10:47:53 PDT
Doing some benchmarking, this doesn't appear to have any perf impact on Linux.  It's solely limited to windows, which makes me think it might just be process overhead and that pymake can save us.
Comment 26 Joshua Cranmer [:jcranmer] 2011-07-18 14:23:31 PDT
Created attachment 546636 [details] [diff] [review]
Synchronize xpidl.py with native xpidl

This patch gets us better parity with xpidl-header, and essentially prints out the same information [IIRC] if you exclude comments (as mentioned earlier, one document comment is off and trying to fudge the output attribute serialization order just isn't worth it).

I could add a few more changes:
1. Modifying the CDATA regex ('(?s)%{[^\n]*\n.*?\n%}[^\n]*$' is what I have in my dxr internal lexer)
2. Handle octal literals properly [I don't see any in anything pulled in by comm-central, so we probably don't care]
3. Add in the other expression operators (^,&,/,%, and unary +,-,~)
4. Initial `_' in identifier

What I don't aim to do in this patch is to replace the native compiler. I'm happy for now to just have a version which I trust to be able to parse the IDL files without errors.
Comment 27 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-07-19 14:44:57 PDT
And I timed it on Windows again and it was at most a 5% slowdown.  Not sure what I did to get teh numbers in comment 22 :-/
Comment 28 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-07-19 15:00:37 PDT
\o/ http://hg.mozilla.org/projects/build-system/rev/85f7679fd5eb
Comment 29 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-07-19 15:57:40 PDT
And a followup for the copy/paste test.

http://hg.mozilla.org/projects/build-system/rev/e5a6f94154c7
Comment 31 Benjamin Smedberg [:bsmedberg] 2011-07-26 09:27:47 PDT
Comment on attachment 546636 [details] [diff] [review]
Synchronize xpidl.py with native xpidl

I'm not sure how much of this is still needed, bug if it is please file a new bug for it to avoid confusion.
Comment 32 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2011-08-12 01:03:20 PDT
Kyle, xpcom/tests still seems to use old xpidl.exe in http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/Makefile.in#196.  Should we keep this or file a new bug?

Note You need to log in before you can comment on or make changes to this bug.