[cfe-dev] Uninitialized Variables Analysis crashing

[João Paulo Labegalini de Carvalho via cfe-dev]

Artem, please find my comments bellow.

Hmm, do we have a backtrace from a debug+assertions build? Like, what are
> we specifically crashing at? It kinda makes sense that we should have the
> variable in the vals list, because we went through its respective DeclStmt,
> right? So i dunno where we are crashing.
>

Its breaking  at "return scratch[idx.getValue()];"
(lib/Analysis/UninitializedValues.cpp at line 213). After some "errs() <<"
debugging I noticed that my VarDecls are note being mapped by
DeclToIndex::computeMap (lib/Analysis/UninitializedValues.cpp at line 78).

Probably I am using the wrong DeclContext then? (I am retrieving it from
SemaRef.CurContext)

I also dumped the AST for the pseudo-code that you provided and i noticed
> more inconsistencies with your AST:
> - Integral cast from int to unsigned is missing within operator ==.
> - The lvalue-to-rvalue around operator & in the __spec_begin call is
> unnecessary because operator & already returns an rvalue.
> - On the other hand, DeclRefExprs for both __setjmp_buf and __setjmp_ret
> must be lvalues.
>

Yes, I had noticed this as well. Its fixed now! I am not doing the Integral
cast, as of now, just to see if it triggers a warning.



> It's also a bit weird that there's no control flow in your CFG. I would
> have expected that a new block is going to start after operator ==, but i
> guess if the user code is unmodified anyway, there probably isn't much
> control flow anyway. But then what's the point of comparing? I'm confused.
>

You are right. However, I am emitting at CGStmt a BranchOnBoolExpr and both
blocks as intended. In the future a intend to reflect the control change in
the AST as well (enabling thus static analysis on my construct).

> One thing I have noticed from the CFG is that all my calls were tagged
> "template".
> CFG just calls Stmt::printPretty() in this case. A quick look at Stmt
> pretty-printing suggests that you have created the DeclRefExpr for the
> function with a non-empty template keyword source location, i.e. this AST
> pretends to be constructed from code
>

Fixed this as well.


>     unsigned int __spec_mode = template __spec_begin(...);
> Also your variables have type 'auto'. And, as far as i understand, we're
> dealing with C code.
>

Also fixed.


> So just to be sure - are you sure you want to auto-generate AST? Did you
> consider implementing your feature as a macro instead, probably with the
> help of a custom compiler builtin for __spec_begin() if it does something
> that a normal function cannot do? I.e., implement all special features in
> the builtin, and instead of auto-generating the surrounding AST, just add a
> macro into the header that expands to whatever you want? It's not as
> omnipotent, but it's much easier. Like, i mean, this AST compiles, but all
> clang tools are screwed unless the AST makes perfect sense; this compiler
> warning is just the first slightly-advanced tool you encountered.
>

I think it makes sense to implement this at the AST level because, in the
future, I intend to add more functionality. For instance, allow the user to
inform which variables do not need to be speculated. Also, once in the AST
I will be able to perform static analysis. But I might be wrong and
appreciate your take on this.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20180926/0b354481/attachment.html>