The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla8

Status

()

Core
Build Config
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: humph, Assigned: khuey)

Tracking

(Depends on: 1 bug, Blocks: 4 bugs)

Trunk
mozilla8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-bs)

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
Created attachment 457144 [details]
IDL Test Case with [deprecated] member
Attachment #457143 - Attachment is obsolete: true
Created attachment 457367 [details] [diff] [review]
PyXPIDL header changes, rev. 1
Attachment #457367 - Flags: review?
Attachment #457367 - Flags: review? → review?(jorendorff)
(Reporter)

Comment 3

7 years ago
Tested this against trunk (Linux), works great!
Blocks: 578790
Comment on attachment 457367 [details] [diff] [review]
PyXPIDL header changes, rev. 1

Turning on pyxpidl is a bold change. Sounds good to me though.
Attachment #457367 - Flags: review?(jorendorff) → review+
Created attachment 461347 [details] [diff] [review]
Create deps from the IDL parser, rev. 1
Attachment #461347 - Flags: review?(me)
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.
Attachment #461347 - Flags: review?(me) → review+
Comment on attachment 457367 [details] [diff] [review]
PyXPIDL header changes, rev. 1

I'm going to take these now, so DXR can be happier.
Attachment #457367 - Flags: approval2.0+
Blocks: 585015
Duplicate of this bug: 584960

Comment 9

7 years ago
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.
Did these land?
Hg says yes.

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

Marking FIXED.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Ah, this got backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #478652 - Flags: review?(benjamin)
That patch causes some of the indexeddb tests to fail.
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?
Attachment #478652 - Attachment is obsolete: true
Attachment #478652 - Flags: review?(benjamin)
(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.
blocking2.0: --- → ?
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.
(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.
blocking2.0: ? → ---
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.
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.
Attachment #485761 - Flags: review?(benjamin)

Updated

6 years ago
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!
Attachment #457367 - Flags: approval2.0+ → approval2.0-
Attachment #485761 - Flags: review?(benjamin) → review+
Assignee: benjamin → khuey
I've updated these patches, but they totally destroy our buildtime on Windows (25% or more in my test).
We should patch bug 585015 and test with it as a pymake native command.
Blocks: 660861
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.
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.
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.
Attachment #542353 - Attachment is obsolete: true
Attachment #546636 - Flags: review?(benjamin)
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 :-/
\o/ http://hg.mozilla.org/projects/build-system/rev/85f7679fd5eb
Whiteboard: fixed-in-bs
And a followup for the copy/paste test.

http://hg.mozilla.org/projects/build-system/rev/e5a6f94154c7
http://hg.mozilla.org/mozilla-central/rev/85f7679fd5eb
http://hg.mozilla.org/mozilla-central/rev/e5a6f94154c7
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
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.
Attachment #546636 - Flags: review?(benjamin)
Depends on: 676231
Depends on: 677395
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?
You need to log in before you can comment on or make changes to this bug.