[Bug target/84272] [8 Regression] AddressSanitizer: heap-use-after-free ../../gcc/config/aarch64/cortex-a57-fma-steering.c:519 in fma_node::get_parity()
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84272 Jakub Jelinek changed: What|Removed |Added Status|ASSIGNED|RESOLVED Resolution|--- |FIXED --- Comment #13 from Jakub Jelinek --- Fixed.
[Bug target/84272] [8 Regression] AddressSanitizer: heap-use-after-free ../../gcc/config/aarch64/cortex-a57-fma-steering.c:519 in fma_node::get_parity()
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84272 --- Comment #12 from Jakub Jelinek --- Author: jakub Date: Fri Feb 16 09:26:27 2018 New Revision: 257727 URL: https://gcc.gnu.org/viewcvs?rev=257727=gcc=rev Log: PR target/84272 * config/aarch64/cortex-a57-fma-steering.c (fma_forest::merge_forest): Use ++iter rather than iter++ for std::list iterators. (func_fma_steering::dfs): Likewise. Don't delete nodes right away, defer deleting them until all nodes in the forest are processed. Do free even leaf nodes. Change to_process into auto_vec. * g++.dg/opt/pr84272.C: New test. Added: trunk/gcc/testsuite/g++.dg/opt/pr84272.C Modified: trunk/gcc/ChangeLog trunk/gcc/config/aarch64/cortex-a57-fma-steering.c trunk/gcc/testsuite/ChangeLog
[Bug target/84272] [8 Regression] AddressSanitizer: heap-use-after-free ../../gcc/config/aarch64/cortex-a57-fma-steering.c:519 in fma_node::get_parity()
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84272 --- Comment #11 from ktkachov at gcc dot gnu.org --- (In reply to Jakub Jelinek from comment #9) > Created attachment 43384 [details] > gcc8-pr84272.patch > > So like this? A few additional changes, Florian Weimer suggested using > preincrement instead of postincrement with the C++ iterators, and I hope > there are no pointers in between different forest, so it should be fine to > free already when processing of the forest is done, rather than waiting > until all forests are processed. > > Could somebody test it on aarch64-linux please? Bootstrap and testing on aarch64-none-linux-gnu with --with-cpu=cortex-a57 showed no problems. Next step is gcc-patches I guess :)
[Bug target/84272] [8 Regression] AddressSanitizer: heap-use-after-free ../../gcc/config/aarch64/cortex-a57-fma-steering.c:519 in fma_node::get_parity()
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84272 --- Comment #10 from Thomas Preud'homme --- (In reply to Jakub Jelinek from comment #9) > Created attachment 43384 [details] > gcc8-pr84272.patch > > So like this? A few additional changes, Florian Weimer suggested using > preincrement instead of postincrement with the C++ iterators, and I hope > there are no pointers in between different forest, so it should be fine to > free already when processing of the forest is done, rather than waiting > until all forests are processed. > > Could somebody test it on aarch64-linux please? Yes I think it should be fine. Sorry, got to head back home again but Kyrill said he would be testing it.
[Bug target/84272] [8 Regression] AddressSanitizer: heap-use-after-free ../../gcc/config/aarch64/cortex-a57-fma-steering.c:519 in fma_node::get_parity()
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84272 Jakub Jelinek changed: What|Removed |Added Attachment #43381|0 |1 is obsolete|| Attachment #43382|0 |1 is obsolete|| Status|NEW |ASSIGNED Assignee|unassigned at gcc dot gnu.org |jakub at gcc dot gnu.org --- Comment #9 from Jakub Jelinek --- Created attachment 43384 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=43384=edit gcc8-pr84272.patch So like this? A few additional changes, Florian Weimer suggested using preincrement instead of postincrement with the C++ iterators, and I hope there are no pointers in between different forest, so it should be fine to free already when processing of the forest is done, rather than waiting until all forests are processed. Could somebody test it on aarch64-linux please?
[Bug target/84272] [8 Regression] AddressSanitizer: heap-use-after-free ../../gcc/config/aarch64/cortex-a57-fma-steering.c:519 in fma_node::get_parity()
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84272
--- Comment #8 from Jakub Jelinek ---
(In reply to Thomas Preud'homme from comment #7)
> Hi Jakub,
>
> First of all, thanks for beating me to it. Wanted to look at it but am
> fighting strong headache since Wednesday.
>
> Regarding the second patch, I believe it is the right approach but found
> another bug in the surrounding lines which I think should be fixed in the
> same patch:
>
> /* Absence of children might indicate an alternate root of a
> *chain*.
> It's ok to skip it here as the chain will be renamed when
> processing the canonical root for that chain. */
> if (node->get_children ()->empty ())
> continue;
>
> Comment 1: I advocate removing this block altogether.
Agreed. After all, what that skips over is:
for (child_iter = node->get_children ()->begin ();
child_iter != node->get_children ()->end (); child_iter++)
to_process.safe_push (*child_iter);
if (free)
{
if (node->root_p ())
delete static_cast (node);
else
delete node;
}
where the for loop will do nothing if node->get_children ()->empty () and the
freeing, which needs to be done too.
> Comment 2: Explain why deferred freeing is necessary on top of the line
> adding the node to to_free.
>
> Something along the lines of:
>
> /* Defer freeing so that the process_node callback can access the parent
> and children of the node being processed. */
Ok.
[Bug target/84272] [8 Regression] AddressSanitizer: heap-use-after-free ../../gcc/config/aarch64/cortex-a57-fma-steering.c:519 in fma_node::get_parity()
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84272 --- Comment #7 from Thomas Preud'homme --- Hi Jakub, First of all, thanks for beating me to it. Wanted to look at it but am fighting strong headache since Wednesday. Regarding the second patch, I believe it is the right approach but found another bug in the surrounding lines which I think should be fixed in the same patch: /* Absence of children might indicate an alternate root of a *chain*. It's ok to skip it here as the chain will be renamed when processing the canonical root for that chain. */ if (node->get_children ()->empty ()) continue; Comment 1: I advocate removing this block altogether. This block prevents node from being deleted when it is a leaf. The comment is also completely outdated and refer to when the dfs was done in the same function as the renaming. Comment 2: Explain why deferred freeing is necessary on top of the line adding the node to to_free. Something along the lines of: /* Defer freeing so that the process_node callback can access the parent and children of the node being processed. */ Best regards, Thomas
[Bug target/84272] [8 Regression] AddressSanitizer: heap-use-after-free ../../gcc/config/aarch64/cortex-a57-fma-steering.c:519 in fma_node::get_parity()
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84272 --- Comment #5 from ktkachov at gcc dot gnu.org --- (In reply to Jakub Jelinek from comment #4) > Created attachment 43382 [details] > gcc8-pr84272-2.patch > > Or defer deletion of all the fma_nodes until the end, whether they are root > or not. I'd fear that even removing just non-root nodes might mean the > following processing of the children might still look at the parent nodes. > > Untested as well. I'd appreciate feedback from the pass author. Thomas (the author of the pass) tells me that he prefers the second approach but he has a few extra comments on the patch that he'll provide later today. In the meantime I can help with testing the patch on aarch64. Thanks for investigating this Jakub. --- Comment #6 from ktkachov at gcc dot gnu.org --- (In reply to Jakub Jelinek from comment #4) > Created attachment 43382 [details] > gcc8-pr84272-2.patch > > Or defer deletion of all the fma_nodes until the end, whether they are root > or not. I'd fear that even removing just non-root nodes might mean the > following processing of the children might still look at the parent nodes. > > Untested as well. I'd appreciate feedback from the pass author. Thomas (the author of the pass) tells me that he prefers the second approach and he has a few extra comments on the patch that he'll provide later today. In the meantime I can help with testing the patch on aarch64. Thanks for investigating this Jakub.
[Bug target/84272] [8 Regression] AddressSanitizer: heap-use-after-free ../../gcc/config/aarch64/cortex-a57-fma-steering.c:519 in fma_node::get_parity()
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84272 --- Comment #5 from ktkachov at gcc dot gnu.org --- (In reply to Jakub Jelinek from comment #4) > Created attachment 43382 [details] > gcc8-pr84272-2.patch > > Or defer deletion of all the fma_nodes until the end, whether they are root > or not. I'd fear that even removing just non-root nodes might mean the > following processing of the children might still look at the parent nodes. > > Untested as well. I'd appreciate feedback from the pass author. Thomas (the author of the pass) tells me that he prefers the second approach but he has a few extra comments on the patch that he'll provide later today. In the meantime I can help with testing the patch on aarch64. Thanks for investigating this Jakub. --- Comment #6 from ktkachov at gcc dot gnu.org --- (In reply to Jakub Jelinek from comment #4) > Created attachment 43382 [details] > gcc8-pr84272-2.patch > > Or defer deletion of all the fma_nodes until the end, whether they are root > or not. I'd fear that even removing just non-root nodes might mean the > following processing of the children might still look at the parent nodes. > > Untested as well. I'd appreciate feedback from the pass author. Thomas (the author of the pass) tells me that he prefers the second approach and he has a few extra comments on the patch that he'll provide later today. In the meantime I can help with testing the patch on aarch64. Thanks for investigating this Jakub.
[Bug target/84272] [8 Regression] AddressSanitizer: heap-use-after-free ../../gcc/config/aarch64/cortex-a57-fma-steering.c:519 in fma_node::get_parity()
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84272 --- Comment #4 from Jakub Jelinek --- Created attachment 43382 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=43382=edit gcc8-pr84272-2.patch Or defer deletion of all the fma_nodes until the end, whether they are root or not. I'd fear that even removing just non-root nodes might mean the following processing of the children might still look at the parent nodes. Untested as well. I'd appreciate feedback from the pass author.
[Bug target/84272] [8 Regression] AddressSanitizer: heap-use-after-free ../../gcc/config/aarch64/cortex-a57-fma-steering.c:519 in fma_node::get_parity()
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84272 --- Comment #3 from Jakub Jelinek --- Created attachment 43381 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=43381=edit gcc8-pr84272.patch Untested patch to defer delete of root nodes.
[Bug target/84272] [8 Regression] AddressSanitizer: heap-use-after-free ../../gcc/config/aarch64/cortex-a57-fma-steering.c:519 in fma_node::get_parity()
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84272 Jakub Jelinek changed: What|Removed |Added CC||jakub at gcc dot gnu.org --- Comment #2 from Jakub Jelinek --- I would have expected that before: if (node->root_p ()) delete static_cast (node); we either process all the children of that root node, or remove the root node and replace it with one of the children.
[Bug target/84272] [8 Regression] AddressSanitizer: heap-use-after-free ../../gcc/config/aarch64/cortex-a57-fma-steering.c:519 in fma_node::get_parity()
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84272 ktkachov at gcc dot gnu.org changed: What|Removed |Added Status|UNCONFIRMED |NEW Last reconfirmed|2018-02-07 00:00:00 |2018-02-08 CC||ktkachov at gcc dot gnu.org, ||thopre01 at gcc dot gnu.org Ever confirmed|0 |1 --- Comment #1 from ktkachov at gcc dot gnu.org --- Confirmed under valgrind.
