compile time regression introduced by Sema checking for __builtin_counted_by_ref

[rapidsna]

As reported by @nikic
#114495 (comment)

This is likely because the checks are currently implemented to walk through the whole child nodes for every single assignments. We may be able to remove the overhead by rolling up the information about usage of counted_by or __builtin_counted_by_ref to the assignment, instead of re-visiting child nodes.

[bwendling]

Could you expand on what you mean by rolling up the information about usage of __builtin_counted_by_ref into the assignment?

[bwendling]

Okay. So what I'd like to do is emit these diagnostics while processing the __builtin_counted_by_ref() call. What I need to do is be able to crawl up the AST from the CallExpr to see how it's used. Is there a way to do that in the front-end?

[nikic]

Maybe @Sirraide knows how to do that?

[bwendling]

I think I found it. You have to use the ParentMapContext to get it. It's so odd...

[Sirraide]

Is there a way to do that in the front-end?

Walking up isn’t really something that’s easy to do in the general case. The only solution I can think of is that it’d be possible to use a placeholder expression for this, but I’m not sure we want to introduce an extra AST node for this (and I don’t think there’s an existing one we could use); that said, maybe there is another way that I’m just not thinking of atm.

Maybe @AaronBallman has an idea?

[rapidsna]

I haven't done it before but could we perhaps put these into some kind of DelayedDiagnostics and then emit it only when certain conditions are met?

[Sirraide]

I haven't done it before but could we perhaps put these into some kind of DelayedDiagnostics and then emit it only when certain conditions are met?

I mean, the problem w/ that is where would you store those, and where would you check for them?

[rapidsna]

If a delayed diagnostics approach won't work then we may need to implement the checking in each place where a pointer can appear as an operand, e.g., binary op (+, -), unary op (*, --, ++), a function call, etc. and prevent it from using the builtin.

[Sirraide]

If a delayed diagnostics approach won't work then we may need to implement the checking in each place where a pointer can appear as an operand,

Yeah, that’s what I was thinking of using a placeholder expression because that’s already checked everywhere

[AaronBallman]

We don't have a cheap way to walk up the AST, only down (walking up involves quite a bit of work).

A placeholder expression could possibly work, but IIRC that involves coming up with a placeholder type that is then used as the type of the operand expression and I don't think we have any such type that maps closely to what we want to do here. Though maybe there is? This call is a sort of reverse-pseudo-object. (Pseudo objects in Clang are used when the syntactic form of the code looks like an object access but the semantic form of the code is a function call; here we have a syntactic form that's a function call but the semantic form is accessing an object.) It's worth investigating whether a placeholder expression would work.

If that doesn't work, then another idea to consider would be an RAII object of some sort. e.g.,

struct DiagnoseBuiltinCountedByRefUses;

class Sema {
  ...
  std::vector<DiagnoseBuiltinCountedByRefUses&> BuiltinCountedByRefTrackers;
  bool isTrackingCountedByRef() const { return !BuiltinCountedByRefTrackers.empty(); }
  DiagnoseBuiltinCountedByRefUses& getCurrentBuiltinCountedByRefTracker() {
    assert(isTrackingCountedByRef()); // Alternatively, we could have a default "tracker" that issues no diagnostics
    return BuiltinCountedByRefTrackers.back();
  }
  ...
};

struct DiagnoseBuiltinCountedByRefUses {
  std::vector<SourceLocation> Locs;
  Sema &Sem;

  DiagnoseBuiltinCountedByRefUses(Sema &S) : Sem(S) {
    Sem.BuiltinCountedByRefTrackers.push_back(*this);
  }
  ~DiagnoseBuiltinCountedByRefUses() {
    llvm::for_each(Locs, [this](SourceLocation Loc) {
      Sem.Diag(Loc, diag::bad_things_have_happened);
    }
    Sem.BuiltinCountedByRefTrackers.pop_back();
  }
};

Then from the various operators you want to guard against using __builtin_counted_by_ref from, you could do:

  {
    DiagnoseBuiltinCountedByRefUses Tracker(TheSemaObject);
    // Continue processing operands of the operator.
  } // Diagnoses any calls to __builtin_counted_by_ref made in an operand.

and from the CallExpr handler where we know we've hit a __builtin_counted_by_ref call, you can do:

if (TheSemaObject.isTrackingCountedByRef())
  TheSemaObject.getCurrentBuiltinCountedByRefTracker().Locs.push_back(TheCallExprLoc);

(You may need additional things to be tracked so you're able to issue a high-quality diagnostic, but it seems like a design like this could work?)

[Sirraide]

IIRC that involves coming up with a placeholder type that is then used as the type of the operand expression

Yeah, the type is what CheckPlaceholderExpr looks at; given that e.g. IncompleteMatrixIdxTy is a thing, I don’t think it’d be too far-fetched to do that for this feature.

This call is a sort of reverse-pseudo-object.

Yeah, that’s also what I was thinking. The main reason I’m proposing a placeholder expression is because we (should) already have machinery in place to check for those everywhere, so it would by default be disallowed everywhere and you’d have to then handle it in all places it’s supported in (which isn’t many for this feature iirc), whereas with other approaches, you’d have to do the reverse, i.e. go to every place we want to disallow the feature in and check for it there, which at a glance seems like a lot more places...

[Sirraide]

but I’m not sure we want to introduce an extra AST node for this

Also, looking at this again, I don’t think we’d need a new AST node—just a new placeholder type would be enough.