1728652 - IDL linkage (idl-analyze.py, ipdl-analyze.rs) needs to compensate for C++ pure virtual declarations now being reported as definitions

Status: RESOLVED FIXED

(Webtools :: Searchfox, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

In bug 1728376 I changed the behavior of C++ pure virtual method declarations to be reported as definitions.

The rationale surrounding this change was:

• In general we do expect a definition to exist for every declaration.
• We explicitly recognize and treat declarations for which isThisDeclarationADefinition() returns true as a definition, not a declaration.
  • We won't also emit a declaration in this case. This is consistent with the searchfox search UI only reporting a given line of code at most once in a set of search results, with lines reported earlier suppressing later lines. (And this goes hand in hand with ordering the results so that definitions precede declarations.)
• We only emit structured analysis records for definitions.
• A pure virtual C++ method declaration will have never have a definition as directly reported by the clang AST.
  • One can conceptually think of it as akin to a specialized form of an inline method definition.

Not surprisingly, this change from emitting declarations to definitions in some cases where there will no longer be a definition can cause breakage in things that assume they will see declarations. This was the case for idl-analyze.py and it looks like ipdl-analyze.rs probably will break too if it gets a chance. There's also an additional level of this being not surprising simply because we don't have test coverage for these directly in the mozsearch tests repo at this time.

I'm going to fix the analyzers now so that they're fine with definitions or declarations provide a source for their mangle-mapping. (They don't know what the full C++ symbol will be so they prefix-match it and were assuming declarations.)

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Attachment: GitHub PR: IDL analyzers should accept defs as well as decls

I've merged this and re-triggered config1 via lambda job.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Thanks for fixing this, and the detailed explanation!