1336589 - IPDL compiler doesn't properly check includes that refer to each other

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Andrew McCreight [:mccr8]]

Consider 3 files, h1.ipdlh, h2.ipdlh and h3.ipdlh. When compiling h1.ipdlh (but not the others!) you get this error message: "error: field `b' of struct `Foo' has unknown type `Bar'"

h1:
include h2;
struct Foo { Bar b; };

h2:
include h3;
struct Bar { bool b; };

h3:
include h1;
struct Baz { bool b; };

I'm not quite sure what is going on, but the handling of mutually recursive includes is a little weird.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Mutually recursive IPDL files that fail.

MozReview-Commit-ID: 3FuXKhgjgGP

Comment 2

[Andrew McCreight [:mccr8]]

If you remove the spurious "include h1" from h3 then the error goes away.

I think what happens is we visitTranslationUnit on h1. The first thing we do there is visitTranslationUnit on h2. The first thing we do there is visitTranslationUnit on h3. This tries to visitTranslationUnit on h1, but we're already visiting them, so nothing happens. Next, we declare the structs for h1 (via visitUsing) and h3. Now, for some reason, there's a chunk of code that goes through the includes for h3 and type checks them all. Note that we haven't declared anything from h2 yet, so we can't find Bar and fail.

I don't think this is particularly worth fixing. Mostly I'm just trying to convince myself that the current approach to type checking is not without its quirks...

Comment 3

[Andrew McCreight [:mccr8]]

I think basically the "second pass" section of visitTranslationUnit that type checks the structs and unions doesn't make much sense:
        for inc in tu.includes:
            if inc.tu.filetype == 'header':
                for su in inc.tu.structsAndUnions:
                    su.accept(self)

You should check a declaration from file foo.ipdlh in the context of foo.ipdlh, not some other random file that happens to include foo. Maybe the idea is that maybe foo.ipdlh wouldn't be checked as a translation unit? Though in that case you probably don't end up doing codegen for it, so you have other problems. This code seems to have been added in the initial landing of ipdlh support (bug 521898) and I don't see any explanation of that part there.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Don't check struct decls from an included file in our context.

This is what the patch is. It makes me a little nervous to just delete a chunk of type checking code, so I may leave it alone.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Bug 1336589 - Don't check struct decls from an included file in the context of the includee.

See MutRecHeader1.ipdlh for a more detailed explanation.

Review commit: https://reviewboard.mozilla.org/r/110832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110832/

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8833496 [details] [diff] [review]
Mutually recursive IPDL files that fail.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=24d9f2d0eba22a0d479e1454c5299f8081af9cff

Comment 7

[Andrew McCreight [:mccr8]]

I thought about it some more, and that chunk of code is just totally bogus, so we should remove it.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8835145 [details]
Bug 1336589 - Don't check struct decls from an included file in the context of the includee.

https://reviewboard.mozilla.org/r/110832/#review112650

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e0ab51985e4
Don't check struct decls from an included file in the context of the includee. r=billm

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e0ab51985e4