Visit nodes in the order of the user output to create better reports

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Visit nodes in the order of the user output to create better reports

Oleg Smolsky via cfe-dev
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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Visit nodes in the order of the user output to create better reports

Oleg Smolsky via cfe-dev
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 <[hidden email]> 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
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Visit nodes in the order of the user output to create better reports

Oleg Smolsky via cfe-dev
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 <[hidden email]> 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 <[hidden email]> 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
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Visit nodes in the order of the user output to create better reports

Oleg Smolsky via cfe-dev
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Visit nodes in the order of the user output to create better reports

Oleg Smolsky via cfe-dev
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Visit nodes in the order of the user output to create better reports

Oleg Smolsky 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev




_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev