[cfe-dev] Uninitialized Variables Analysis crashing

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

I was thinking one giving up on this, but then I remembered that I need
finish this to actually move on with my PhD. =<

Just in case someone makes the same mistakes I made, I am replying to
register how i solved it.

The actual bug was caused by the DeclVars inside the CompoundStmt I was
transforming through TreeTransform. I have figured out that my "->dump()"
debugging was misleading. As DeclVars were not been added to the current
context via SemaRef.CurContex->addDecl(Decl*), the analysis was trying ti
access uninitialized memory and thus triggering a segfault. Once I added
the addDecl calls the analysis could be left enabled (default) and the
compilation proceeds gracefully.

I thank you Artem once again for your time and help.
Hopefully in the future I might be able to give back by helping somebody
else.

On Wed, Sep 26, 2018 at 6:17 PM Artem Dergachev <noqnoqneo at gmail.com> wrote:

> The correct DeclContext for these variables is the definition of the
> function (i.e., the re-declaration that has a body).
>
> Your DeclContext looks correct because we know that the given DeclRefExpr
> has been classified as Use, which can only happen when isTrackedVar()
> return true - cf. findVar().
>
> But for the same reason they should not be skipped in computeMap(). Weird.
>
>
>
> On 9/26/18 1:40 PM, João Paulo Labegalini de Carvalho wrote:
>
> 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.
>
>
>
>

-- 
João Paulo L. de Carvalho
Computer Science |  IC-UNICAMP | Campinas , SP - Brazil
jaopaulolc at gmail.com
joao.carvalho at ic.unicamp.br
j160924 at dac.unicamp.br
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20181111/b0f09dee/attachment.html>