[clang] Diagnose overly complex Record in __builtin_dump_struct

[dblaikie]

Hopefully my clarification on https://reviews.llvm.org/D154784#inline-1531176 makes it clear that assertions aren't necessarily the right tool here - as this path is reachable with actual code & I don't think we should allow that code to reach UB in the compiler in a non-assertionsn build. If we think this is a sufficient size to support, then we should error clearly even in a non-assertions build.

[yronglin]

Addres the comments that we talked in D154784.

[aaron.ballman]

The changes generally LGTM, thank you!

clang/lib/Sema/SemaChecking.cpp

732–733

This will correctly handle diagnosing a gigantic anonymous struct.

[rsmith]

clang/lib/Sema/SemaChecking.cpp

732–733

Producing an error here seems likely to eventually cause problems in practice for some users: people are using __builtin_dump_struct in generic code for reflection purposes, not just for debugging, and this will cause us to start rejecting complex generic code.

Instead of rejecting, can we produce a tree of PseudoObjectExprs if we have too many steps to store in a single expression?

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

732–733

Producing an error here seems likely to eventually cause problems in practice for some users: people are using __builtin_dump_struct in generic code for reflection purposes, not just for debugging, and this will cause us to start rejecting complex generic code.

Instead of rejecting, can we produce a tree of PseudoObjectExprs if we have too many steps to store in a single expression?

I think that requires wider discussion -- I don't think __builtin_dump_struct is a reasonable interface we want to support for reflection (in fact, I'd argue it's an explicit non-goal, the same as reflection via -ast-dump). Compile-time reflection is something we're likely to need to support more intentionally and I don't think we're going to want to use this as an interface for it or have to maintain it as a reflection tool long-term. As such, I think producing a tree of PseudoObjectExprs is overkill; you can quote me on this a few years from now when we're laughing at its quaintness, but "16k fields of debug output is enough for anyone" for a debugging interface.

(That said, I think we should be considering what support we want to add to the compiler for reflection in service of the work being done in WG21 on the topic -- if __builtin_dump_struct is being used for reflection in practice, it would be nice to give people a supported, more ergonomic interface for it that we can use for a future version of C++.)

[rsmith]

clang/lib/Sema/SemaChecking.cpp

732–733

The bug report https://github.com/llvm/llvm-project/issues/63169 was encountered by a user hitting the previous 256-element limit in practice when using __builtin_dump_struct for reflection. I don't think we can reasonably prevent that from happening, other than -- as you say -- encouraging WG21 to give us a real reflection design we can implement.

[yronglin]

Address Aaron's comments.

[yronglin]

Thank you for your review @aaron.ballman @rsmith , I will be happy to continue cook this patch once we reach a consensus.

clang/lib/Sema/SemaChecking.cpp

732–733

fixed.

[dblaikie]

clang/lib/Sema/SemaChecking.cpp

732–733

@rsmith what do you think we should do now? Support even larger values than uint16_t worth? (the tree PseudoObjectExpr suggestion you made?)

If we did make the uint16_t limit stand - perhaps that'd apply pressure to folks who have been using this for RTTI to consider other options - it wouldn't break any existing code (that's not already broken in clang, at least - with the overflow stuff happening) but might put pressure on further growth of such techniques?

[dblaikie]

@rsmith ping on this thread - wondering what you'd prefer the direction be here

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_builtin_longjmp_unsupported : Error<
   "__builtin_longjmp is not supported for the current target">;
 def err_builtin_setjmp_unsupported : Error<
   "__builtin_setjmp is not supported for the current target">;
 
 def err_builtin_longjmp_invalid_val : Error<
   "argument to __builtin_longjmp must be a constant 1">;
 def err_builtin_requires_language : Error<"'%0' is only available in %1">;
 
+def err_builtin_dump_struct_too_complex : Error<
+"%select{class|struct|union}0 '%1' is too complex to dump">;
+
 def err_constant_integer_arg_type : Error<
   "argument to %0 must be a constant integer">;
 
 def ext_mixed_decls_code : Extension<
   "mixing declarations and code is a C99 extension">,
   InGroup<DeclarationAfterStatement>;
 def warn_mixed_decls_code : Warning<
   "mixing declarations and code is incompatible with standards before C99">,

clang/lib/Sema/SemaChecking.cpp

static ExprResult SemaBuiltinDumpStruct(Sema &S, CallExpr *TheCall) {
   // incorrect '&s->n'.
   Expr *PtrArg = PtrArgResult.get();
   PtrArg = new (S.Context)
       ParenExpr(PtrArg->getBeginLoc(),
                 S.getLocForEndOfToken(PtrArg->getEndLoc()), PtrArg);
   if (Generator.dumpUnnamedRecord(RD, PtrArg, 0))
     return ExprError();
 
+  // We create a `PseudoObjectExpr` as a wrapper, but the
+  // `PseudoObjectExprBits.NumSubExprs` in `PseudoObjectExpr` restricts its
+  // value to no more than std::numeric_limits<uint16_t>::max().
+  if (Generator.Actions.size() > std::numeric_limits<uint16_t>::max()) {
+    int RDKind = RD->isClass() ? 0 : (RD->isStruct() ? 1 : 2);
+    S.Diag(PtrArg->getBeginLoc(), diag::err_builtin_dump_struct_too_complex)
+        << RDKind << RD->getName();
+    return ExprError();
+  }
+
   return Generator.buildWrapper();
 }
 
 static bool SemaBuiltinCallWithStaticChain(Sema &S, CallExpr *BuiltinCall) {
   if (checkArgCount(S, BuiltinCall, 2))
     return true;
 
   SourceLocation BuiltinLoc = BuiltinCall->getBeginLoc();

clang/test/SemaCXX/builtin-dump-struct.cpp

t1 v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
       v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
       v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
       v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
       v92, v93, v94, v95, v96, v97, v98, v99;
 };
 
 int printf(const char *, ...);
 void f1(t2 w) { __builtin_dump_struct(&w, printf); }
+
+struct t3 { };
+template<typename T1>
+struct templ {
+    T1 v1;
+    T1 v2;
+    T1 v3;
+    T1 v4;
+};
+
+struct t4 {
+  templ<templ<templ<templ<templ<templ<t3>>>>>> c0;
+  templ<templ<templ<templ<templ<templ<t3>>>>>> c1;
+  templ<templ<templ<templ<templ<templ<t3>>>>>> c2;
+};
+
+void aj(...);
+void f2(t4 w) { __builtin_dump_struct(&w, aj); } // expected-error{{struct 't4' is too complex to dump}}
+