[PATCH] D150892: [clang][ExprConstant] fix __builtin_object_size for flexible array members

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:11740
+          LVal.getLValueBase().dyn_cast<const ValueDecl *>());
+      Result += VD->getFlexibleArrayInitChars(Info.Ctx);
+    }
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > erichkeane wrote:
> > > nickdesaulniers wrote:
> > > > erichkeane wrote:
> > > > > Isn't this a possible null-deref?  
> > > > I don't think so; in fact, I can use `cast` and `get` rather than `dyn_cast_or_null` and `dyn_cast` here.
> > > > 
> > > > Just because we have a pointer doesn't mean it's possibly `nullptr`; I don't think we can reach this code patch for evaluating the `__builtin_object_size` of a struct with a flexible array member if the LValue doesn't have a corresponding VarDecl.
> > > Of course that is a possibility.  You shouldn't use dyn_cast right before a dereference, this should definitely be using `cast`/`get`, since they assert.
> > > 
> > > What does the LValue have when the dynamic struct does not have an initializer?  Or just an empty one? 
> > s/code patch/code path/
> > What does the LValue have when the dynamic struct does not have an initializer? Or just an empty one?
> 
> Tested:
> ```
> struct foo {
>     int x;
>     int y[];
> };
> 
> static struct foo instance = {
>     .x = 3,
>     // .y = { 5, 10, 15, },
> };
> 
> unsigned long foo (void) {
>     return __builtin_object_size(&instance, 1);
> }
> ```
> With the following values for `.y` in `instance`:
> 1. `.y = { 5, 10, 15 }`: `foo` returns `16`.
> 2. `.y = {},`: `foo` returns `4`.
> 3. <`.y` is not specified or explicitly initialized>: `foo` returns `4`.
This might have been missed in the discussion here, but what actually ensures we have a VarDecl here?  I guess most other LValueBases can't actually have a struct type, but a CompoundLiteralExpr can. (Looking briefly at the code, I can't find any other possibilities at the moment, but in any case, the assumption seems pretty fragile.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150892