Closed Bug 1336589 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: IPC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: mccr8, Unassigned)

Details

Attachments

(1 file, 2 obsolete files)

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.
MozReview-Commit-ID: 3FuXKhgjgGP
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...
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.
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.
Attachment #8833536 - Attachment is obsolete: true
I thought about it some more, and that chunk of code is just totally bogus, so we should remove it.
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
Attachment #8835145 - Flags: review?(wmccloskey) → review+
Pushed by amccreight@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/6e0ab51985e4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.