1947308 - Searchfox shape field layout view can be confused by base classes

Status: NEW

(Webtools :: Searchfox, enhancement)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

See https://searchfox.org/mozilla-central/query/default?q=field-layout%3A%27js%3A%3AShape%27: There' no indication of the base class which is gc::CellWithTenuredGCPointer<gc::TenuredCell, BaseShape>

Comment 1

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

We know the superclass is symbol T_js::gc::CellWithTenuredGCPointer as can be seen in the inheritance diagram but we don't emit a structured record for that type, which is also why the inheritance diagram doesn't have a pretty identifier for the base class. I think I identified this in bug 1730052 but since this bug has a concrete use-case which is likely of interest to a bunch of people, we should keep this bug open for the specific use-case.

It's possible this check in VisitNamedDecl that skips calling emitStructuredInfo is part of what is making us sad and we can relax things so that we only call getASTRecordLayout for instantiations where we would actually have a layout:

if (D2->isThisDeclarationADefinition() &&
    // XXX getASTRecordLayout doesn't work for dependent types, so we
    // avoid calling into emitStructuredInfo for now if there's a
    // dependent type or if we're in any kind of template context.  This
    // should be re-evaluated once this is working for normal classes and
    // we can better evaluate what is useful.
    !D2->isDependentType() && !TemplateStack) {

The other thing is that we currently do intentionally truncate our concept of a type/class such that we ignore the parameters and this generally is important to avoid overwhelming searchfox with concrete instantiations when the instantiations don't matter. For the case of structured records I don't think this is likely to actually be a problem, so we could probably change our behavior to emit structured records for all of the template instantiations and make sure to have the superclass/subclass relationships use these more specific symbols and corresponding pretty identifiers.

An interesting question is whether CellWithTenuredGCPointer<gc::TenuredCell, BaseShape> should be a subclass of CellWithTenuredGCPointer<class BaseCell, class PtrT> or if there should be explicit binding slot relationship or another type of new explicit edge. I've been leaning towards the latter two options. My only abiding thought in this area was that for general processing purposes it would be nice to have crossref at least aggregate sets of template parameter values that end up being logically equivalent so that we could effectively create a phylogenetic tree-ish visualization for templates to help understand how their implementation works and varies over different parameters.

Comment 2

[Tooru Fujisawa [:arai]]

The getASTRecordLayout can be called for the ClassTemplateSpecializationDecl class, which can be collected by traversing the CXXRecordDecl::bases().
At least locally, calling emitStructuredRecordInfo for each base works for template cases.

Then, given the instantiation/specialization is associated to the consumer, instead of the base class's declaration, the possible option I can think of is to emit the structured record inside the subclass's record, for each occurrences.
So that the subclass's field layout can visualize the base classes for the specific template parameters, by slightly tweaking the field layout code.
This information can be limited only to the templatized base classes, so that other cases aren't affected.

Comment 3

[Tooru Fujisawa [:arai]]

I'm experimenting with something like the following:

{
  "loc": "00013:7-10",
  "structured": 1,
  "pretty": "field_layout::template_base::Sub",
  "sym": "T_field_layout::template_base::Sub",
  "kind": "struct",
  "sizeBytes": 3,
  "alignmentBytes": 1,

  "supers": [
    {
      "sym": "T_field_layout::template_base::Base",
      "offsetBytes": 0,
      "props": [],

      // A field added when the base class is templatized
      "specialization": {
        "sizeBytes": 2,
        "alignmentBytes": 1,

        // Somehow stringify the template arguments?
        "templateArguments": [
          "uint8_t",
          "uint16_t"
        ],

        // Base class fields.
        "fields": [
          {
            "lineRange": "#9",
            "pretty": "field_layout::template_base::Base::a",
            "sym": "F_<T_field_layout::template_base::Base>_a",
            "type": "unsigned char",
            "offsetBytes": 0,
            "sizeBytes": 1
          },
          ...
        ]
      }
    }
  ],

  // Sub class fields.
  "fields": [
    {
      "lineRange": "#14",
      "pretty": "field_layout::template_base::Sub::c",
      "sym": "F_<T_field_layout::template_base::Sub>_c",
      "type": "unsigned char",
      "offsetBytes": 2,
      "sizeBytes": 1
    }
  ]
}

or perhaps we could generate the base class's record without layout as singleton, and put per-layout data into each instantiation/specialization, to reduce the duplication.