578478 - xpidl.py can't parse [deprecated] attributes/methods

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[David Humphrey (:humph)]

Attachment: 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)]

Attachment: IDL Test Case with [deprecated] member

Comment 2

[Benjamin Smedberg]

Attachment: PyXPIDL header changes, rev. 1

Comment 3

[David Humphrey (:humph)]

Tested this against trunk (Linux), works great!

Comment 4

[Jason Orendorff [:jorendorff]]

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]

Attachment: Create deps from the IDL parser, rev. 1

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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]

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 9

[Walter Meinl]

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 (Exited; not receiving bugmail, old account, do not use)]

Did these land?

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Hg says yes.

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

Marking FIXED.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ah, this got backed out.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: 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 (Exited; not receiving bugmail, old account, do not use)]

That patch causes some of the indexeddb tests to fail.

Comment 15

[Benjamin Smedberg]

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 (Exited; not receiving bugmail, old account, do not use)]

(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]

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 (Exited; not receiving bugmail, old account, do not use)]

(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]

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 (Exited; not receiving bugmail, old account, do not use)]

Attachment: 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]]

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 (Exited; not receiving bugmail, old account, do not use)]

I've updated these patches, but they totally destroy our buildtime on Windows (25% or more in my test).

Comment 23

[(not currently active) Ted Mielczarek]

We should patch bug 585015 and test with it as a pymake native command.

Comment 24

[Joshua Cranmer [:jcranmer]]

Attachment: 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 (Exited; not receiving bugmail, old account, do not use)]

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]]

Attachment: 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 (Exited; not receiving bugmail, old account, do not use)]

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 (Exited; not receiving bugmail, old account, do not use)]

\o/ http://hg.mozilla.org/projects/build-system/rev/85f7679fd5eb

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

And a followup for the copy/paste test.

http://hg.mozilla.org/projects/build-system/rev/e5a6f94154c7

Comment 30

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/85f7679fd5eb
http://hg.mozilla.org/mozilla-central/rev/e5a6f94154c7

Comment 31

[Benjamin Smedberg]

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]]

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?