[AST] Assert that FoldingSetNodeID used for hint is correct upon insertion

[ilya-biryukov]

No description provided.

[ilya-biryukov]

Sharing this early to showcase the approach and see if folks are on board with that.
The approach adds some boilerplate and requires discipline, but I couldn't come up with a different one that avoids that.

I have seen some failures (~150) with ninja check-clang, will investigate and fix before sending this out for a proper review.

[mizvekov]

I think having to call a separate thing at every use site is a bit prone to error. Can't we just check this at InsertNode instead?

[ilya-biryukov]

I think having to call a separate thing at every use site is a bit prone to error.

I fully agree. "requires discipline" in my previous comment alludes to that exactly.

Can't we just check this at InsertNode instead?

I initially thought it's not possible because we don't have the ID there, but I think you're right and we could just try to recompute InsertPos and check it's the same as suggested. I'll try that approach and update this PR.

[mizvekov]

I initially thought it's not possible because we don't have the ID there, but I think you're right and we could just try to recompute InsertPos and check it's the same as suggested. I'll try that approach and update this PR.

Right, though just checking we have the same InsertPos would weaken the assertion.

What about doing it in FindNodeOrInsertPos. In debug build, we can check if we actually found the node, in that case we check the ID is the same.

[mizvekov]

What about doing it in FindNodeOrInsertPos. In debug build, we can check if we actually found the node, in that case we check the ID is the same.

Err, that doesn't actually buys us much. Yeah I don't have a better idea right now.

[mizvekov]

Best I can come up, is that we start accepting the ID in InsertNode, but not do anything with it in release builds.

If we discard this ID in an inline function, that shouldn't cause performance regressions.

Then we can check the InsertPos is correct as well, which would improve things.

[erichkeane]

What if we replace the calls to insertNode with a helper function macro instead? So:

InsertNode(VectorTypes, ID, New, InsertPos);

Which is then:
#define InsertNode(Ty, ID, Elt, Pos) assert(<call to helper>);Ty.InsertNode(Ty, Pos);

Sensible? Or presumably we can have an always-inline function wrapper for that, and the inliner will just take care of it.

[mizvekov]

Sensible? Or presumably we can have an always-inline function wrapper for that, and the inliner will just take care of it.

Yeah, I think the inline helper is cleaner, we can just discard ID before calling anything not inline.

[erichkeane]

Sensible? Or presumably we can have an always-inline function wrapper for that, and the inliner will just take care of it.

Yeah, I think the inline helper is cleaner, we can just discard ID before calling anything not inline.

Sounds good to me, an always_inline tagged one (there is a LLVM flag for it?!) would probably have no overhead as a result.

[ilya-biryukov]

+1 for the inline helper. But if we go down that path, maybe we could add an optional parameter to InsertNode and make it the helper we're talking about?

[[always_inline]] Ret InsertNode(<args>, llvm::FoldingSetNodeID* IDForAssert = nullptr) {
  assert(!IDForAssert || IDForAssert == ...);
  return InsertNodeImpl(<args>);
}

[erichkeane]

+1 for the inline helper. But if we go down that path, maybe we could add an optional parameter to InsertNode and make it the helper we're talking about?

[[always_inline]] Ret InsertNode(<args>, llvm::FoldingSetNodeID* IDForAssert = nullptr) {
  assert(!IDForAssert || IDForAssert == ...);
  return InsertNodeImpl(<args>);
}

That would be acceptable as well to me.

[mizvekov]

Yeah, I did include that possibility implicitly in my suggestion, thanks.