[PATCH] D80981: [AST] Fix a crash on accessing a class without definition in constexpr function context.

Richard Smith - zygoloid via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jun 24 14:40:44 PDT 2020


rsmith accepted this revision.
rsmith marked an inline comment as done.
rsmith added a comment.
This revision is now accepted and ready to land.

Minor suggestions but this LGTM.



================
Comment at: clang/lib/AST/ExprConstant.cpp:4320
+    if (!RD->hasDefinition())
+      return APValue();
     APValue Struct(APValue::UninitStruct(), RD->getNumBases(),
----------------
hokein wrote:
> rsmith wrote:
> > hokein wrote:
> > > rsmith wrote:
> > > > hokein wrote:
> > > > > sammccall wrote:
> > > > > > This doesn't look all that safe - you're using a `None` value to indicate failure, but no current code path does that and none of the callers seem to check for failure.
> > > > > > (e.g. `evaluateVarDecl` returns true instead of false).
> > > > > > Presumably we're going to get a diagnostic somewhere (though it's not completely obvious to me) but can we be sure we won't assume this value has the right type somewhere down the line?
> > > > > > 
> > > > > > I get the feeling this is correct and I don't have enough context to understand why... how about you :-)
> > > > > I don't have a promising explanation neither. 
> > > > > 
> > > > > I didn't find a better way to model failures in `getDefaultInitValue`. This function is used in multiple places of this file (and I'm not sure whether we should change it).
> > > > > 
> > > > > @rsmith any thoughts?
> > > > `APValue()` is a valid representation for an object of class type, representing a class object that is outside its lifetime, so I think it's OK to use this representation, if we can be sure that this only happens along error paths. (It's not ideal, though.)
> > > > 
> > > > If we can't be sure this happens only along error paths, then we should produce a diagnostic here. Perhaps feed an `EvalInfo&` and a source location into every caller of this function and produce a diagnostic if we end up querying the default-initialized value of an incomplete-but-valid class type. Or perhaps we could check that the class is complete and valid from every caller of this function instead. (I think that we guarantee that, for a valid complete class type, all transitive subobjects are of valid complete types, so checking this only once at the top level before calling into `getDefaultInitValue` should be enough.)
> > > Thanks for the suggestions.
> > > 
> > > oh, yeah, I missed that the `Foo` Decl is invalid, so checking the class decl is valid at every caller of `getDefaultInitValue` should work -- it would also fix other potential issues, looks like here we guarantee that the VarDecl is valid, but don't verify the decl which the VarDecl's type refers to is valid in all callers.  
> > > 
> > > Given the fact that the `VarDecl` e is valid and class `Foo` Decl is invalid, another option to fix the crash is to invalidate this `VarDecl`. Should we invalidate the VarDecl if the type of the VarDecl refers to an invalid decl? My gut feeling is that it is worth keeping the VarDecl valid, so that more related AST nodes will be built (for better recovery and diagnostics), though it seems unsafe. 
> > I think keeping the `VarDecl` valid is probably the better choice, to allow us to build downstream uses of it. Also, because variables can be redeclared, we could have something like `struct A; extern A v; struct A { invalid; };` -- and we can't reasonably retroactively mark `v` as invalid in this case, so we can't guarantee that the type of every valid variable is itself valid. (We *could* guarantee that the type of every valid variable *definition* is valid, but that will lead to inconsistencies where defining the variable causes later behavior of references to the variable to change.)
> > 
> > It's really unfortunate that we don't have a good definition of what "valid" means for a variable, or really any listing of what invariants we maintain in the AST in the presence of invalid nodes. :( This is one of the things I would work on if I had time...
> > I think keeping the VarDecl valid is probably the better choice, to allow us to build downstream uses of it. Also, because variables can be redeclared, we could have something like struct A; extern A v; struct A { invalid; }; -- and we can't reasonably retroactively mark v as invalid in this case, so we can't guarantee that the type of every valid variable is itself valid. (We *could* guarantee that the type of every valid variable *definition* is valid, but that will lead to inconsistencies where defining the variable causes later behavior of references to the variable to change.
> 
> yeah, that makes sense, thanks for the explanation.
> 
> I have updated the patch -- now the `getDefaultInitValue()` does error check. If fails, return `APValue()` which will only happen on error paths. Since it changes non-trivial amount of code, would be nice if you can take a look.
> 
> > It's really unfortunate that we don't have a good definition of what "valid" means for a variable, or really any listing of what invariants we maintain in the AST in the presence of invalid nodes. :( This is one of the things I would work on if I had time...
> 
> that would be nice to have, and given that we have containsErrors, the meanings of them are subtle (sometimes I got confused by these concepts). Would you like me to help here? happy to help though I don't fully understand clang yet.
> 
> Would you like me to help here? happy to help though I don't fully understand clang yet.

If you're motivated, this might be a good way to learn more about Clang :) You'll certainly discover things that no-one knows about Clang (and a fair few bugs) if you add something like the LLVM IR verifier for the Clang AST.


================
Comment at: clang/lib/AST/ExprConstant.cpp:4315
 /// Get the value to use for a default-initialized object of type T.
-static APValue getDefaultInitValue(QualType T) {
+/// Return false if fails.
+static bool getDefaultInitValue(QualType T, APValue &Result) {
----------------
It would be useful to mention here that this only happens if we encounter something invalid (so that callers know they don't need to produce a nice diagnostic).


================
Comment at: clang/lib/AST/ExprConstant.cpp:5783-5787
+          else if (!getDefaultInitValue(Info.Ctx.getRecordType(CD), *Value))
+            // FIXME: This immediately starts the lifetime of all members of
+            // an anonymous struct. It would be preferable to strictly start
+            // member lifetime in initialization order.
+            Success = false;
----------------
Nit: you're inconsistently using `if (!get) Success = false;` here and `Success &= get;` above and below. I'd prefer it if you expressed this the same way in all three cases in this function.


================
Comment at: clang/lib/AST/ExprConstant.cpp:8923
+  } else if (!getDefaultInitValue(AllocType, *Val)) {
+    return Error(E);
   }
----------------
This is the only place where we produce a diagnostic after `getDefaultInitValue` fails. It'd be more consistent to just `return false` here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80981/new/

https://reviews.llvm.org/D80981





More information about the cfe-commits mailing list