[cfe-dev] Visit nodes in the order of the user output to create better reports

[George Karpenkov via cfe-dev]

Yes, changing BugReporter.cpp is a very non-trivial task,
and should generally be avoided.

> On Nov 16, 2018, at 10:48 AM, Csaba Dabis <dabis.csaba98 at gmail.com> wrote:
> 
> Thanks you! This is a very good idea. I just wanted to rewrite the root of the visiting to prevent problems like that in the future, as the following (in BugReporter.cpp):
> 
> static std::unique_ptr<VisitorsDiagnosticsTy>
> generateVisitorsDiagnostics(BugReport *R, const ExplodedNode *ErrorNode,
>                             BugReporterContext &BRC) {
>   typedef std::vector<const ExplodedNode *> NodePath;
> 
>   auto Notes = llvm::make_unique<VisitorsDiagnosticsTy>();
>   BugReport::VisitorList visitors;
>   NodePath Path;
> 
>   // Run visitors on all nodes starting from the node *before* the last one.
>   // The last node is reserved for notes generated with {@code getEndPath}.
>   const ExplodedNode *Pred = ErrorNode;
>   while (Pred) {
>     Pred = Pred->getFirstPred();
>     if (!Pred)
>       break;
> 
>     if (!R->isValid())
>       break;
> 
>     Path.push_back(Pred);
>   }
> 
>   for (NodePath::reverse_iterator I = Path.rbegin(); I != Path.rend(); ++I) {
>     const ExplodedNode *NextNode = *I;
>     
>     // At each iteration, move all visitors from report to visitor list.
>     for (BugReport::visitor_iterator VI = R->visitor_begin(),
>                                      E = R->visitor_end();
>          VI != E; ++VI) {
>       visitors.push_back(std::move(*VI));
>     }
>     R->clearVisitors();
> 
>     for (auto &V : visitors) {
>       llvm::errs() << "\nbefore crash - it is written out";
>       auto P = V->VisitNode(NextNode, BRC, *R);
>       llvm::errs() << "\nafter crash";
>       if (P)
>         (*Notes)[NextNode].push_back(std::move(P));
>     }
> 
>     if (!R->isValid())
>       break;
>   }
> 
>   std::shared_ptr<PathDiagnosticPiece> LastPiece;
>   for (auto &V : visitors) {
>     V->finalizeVisitor(BRC, ErrorNode, *R);
> 
>     if (auto Piece = V->getEndPath(BRC, ErrorNode, *R)) {
>       assert(!LastPiece &&
>              "There can only be one final piece in a diagnostic.");
>       LastPiece = std::move(Piece);
>       (*Notes)[ErrorNode].push_back(LastPiece);
>     }
>   }
> 
>   return Notes;
> }
> 
> I think you are right about it I should change things locally, so I will drop that patch, because it is too difficult for me.
> 
> George Karpenkov <ekarpenkov at apple.com <mailto:ekarpenkov at apple.com>> ezt írta (időpont: 2018. nov. 16., P, 19:25):
> I’m still not exactly sure what are you doing,
> but you can store all the information you need in a vector inside the visitor,
> and then iterate over it in the callback for the last node.
> Is that what you’ve tried originally? Where does it crash?
> 
>> On Nov 16, 2018, at 10:22 AM, Csaba Dabis <dabis.csaba98 at gmail.com <mailto:dabis.csaba98 at gmail.com>> wrote:
>> 
>> The current way of bug reporting:
>> ---
>> 1 Assuming 'i' is not equal to 1
>> ----
>> 2 Assuming 'i' is not equal to 2
>> 3 Taking false branch
>> 4 Taking false branch
>> ---
>> 
>> If I would like to hook extra information in the linear backwards way, this is going to be like the following:
>> ---
>> 1 Assuming 'i' is not equal to 1
>> ----
>> 2 Knowing 'i' is not equal to 2
>> 3 Knowing 'i' is not equal to 1
>> 4 Assuming 'i' is not equal to [1, 2]
>> ---
>> 
>> And actually the proper approach would be:
>> ---
>> 1 Assuming 'i' is not equal to 1
>> ---
>> 2 Assuming 'i' is not equal to 2
>> 3 Knowing 'i' is not equal to [1, 2]
>> 4 Knowing 'i' is not equal to [1, 2]
>> ---
>> 
>> George Karpenkov <ekarpenkov at apple.com <mailto:ekarpenkov at apple.com>> ezt írta (időpont: 2018. nov. 16., P, 19:04):
>> Going forward is ambiguous, as there are many leafs, but only one root.
>> What are you trying to achieve? Going backwards should never be a problem.
>> 
>> > On Nov 16, 2018, at 8:21 AM, Csaba Dabis via cfe-dev <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>> wrote:
>> > 
>> > Hello!
>> > 
>> > Why does the 'generateVisitorsDiagnostics()' function in BugReporter.cpp work backwards? It starts from the node of the error and iterate over the preds.
>> > 
>> > I have tried to make it to work forward with storing the 'const ExplodedNode *' nodes in a vector, but 'auto P = V->VisitNode(NextNode, BRC, *R);' crashes. I just found out the 'LastPiece' checking could be moved out from the while-loop, that is it for now.
>> > 
>> > Is it possible to achieve? What is could be problematic with that code snippet?
>> > 
>> > Thanks you for the suggestions,
>> > Csaba.
>> > _______________________________________________
>> > cfe-dev mailing list
>> > cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev>
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20181116/88819fd7/attachment.html>