436332 - Require attribute annotation on forward declarations based on the complete type

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Benjamin Smedberg]

Many XPCOM interfaces and classes are pre-declared incomplete classes. In order for many of the XPCOMGC analyses to work correctly, we need to know whether an incomplete type is a GC type or not, e.g.:

__attribute__((user("NS_GC"))) class nsIFoo;

I want my analysis to *require* that forward declarations of GC classes have annotations, so that the following code produces an error:

class nsIFoo;

class nsIFoo : public nsISupports { // nsISupports is NS_GC, so nsIFoo is also
  virtual...
};

Any thoughts on how this can be accomplished?

Comment 1

[(dormant account)]

That's a weird one. Forward declarations aren't too pleasant to work with. I think your best bet is to detect incomplete types(by scanning members or something?) and then pattern match to ensure that incomplete nsI* types have your attributes.

Or perhaps you just want to do this at class declaration time..and in process_type ensure that it has the right attribute. Of course GCC is messy with how it deals with forward declarations so this could require further gcc patching.

Comment 2

[Benjamin Smedberg]

Well right now the problem is, by the time I see the types in treehydra, if they are resolved in this translation unit, they already are.

So there's only two states:
* If the class remains incomplete, I see the incomplete type
* If the class is completed, I see the complete type

There isn't ever an in-between state where I can first see the incomplete type and later the complete type.

Comment 3

[David Mandelin [:dmandelin]]

The original requirement seems Dehydra-like, in the sense that it only matters for Dehydra analyses. In Treehydra you always see the "last" declaration/definition, so it won't matter in Treehydra. Right? If so, that makes me think you would want to check this using Dehydra process_type.

Comment 4

[Benjamin Smedberg]

So, what about this... can I get process_type called multiple times, first with the incomplete type and then the complete type?

Comment 5

[(dormant account)]

Not without mods to GCC. 

Comment 6

[David Mandelin [:dmandelin]]

Ugh. This is painful. But let me see if I understand the actual problem. I think what you are saying is that you might have foo.cpp include qux.h:

  # foo.ii
  # from qux.h
  class nsIBar;
  # from foo.cpp
  // If nsIBar is an NS_GC class, we don't know it and we'll do the wrong thing
  class nsIFoo : public nsIBar {

Where presumably bar.cpp preprocesses to something like this:

  # bar.ii
  # from qux.h
  class nsIBar;
  # from bar.cpp
  // Here, we know nsIBar must be NS_GC, so we know the fwd decl is lying.
  class nsIBar : public nsISupports {

I think algorithm doesn't necessarily detect all lying forwards--it only detects lying forwards that get included into the same file with a definition at some point. Or do you know that this is true?

Anyway, a more reliable way to do this, that also works in current GCC, would be to log the NS_GC status assigned to each class in each compilation unit. Then at the end see if there are any disagreements. I know this has the disadvantage of requiring a separate pass outside the per-file analyses, but it should not be too complex and should scale ok. I guess you also have to figure out where to put those logs.

Unless I'm mistaken, the property you want to check is fundamentally a cross-compilation-unit property, of the sort that would traditionally be detected (or "detected") by a linker, so I'd really recommend going that way.

But if you really do need to see forwards explicitly, we currently use a patch that touches related code in GCC, so we have some idea how to do it at least.

Comment 7

[Joshua Cranmer [:jcranmer]]

Dehydra and treehydra are no longer maintained by Mozilla.