Nicolai Hähnle via llvm-dev
2016-Oct-06 11:54 UTC
[llvm-dev] Adding asan poison to Recycler and ArrayRecycler
Hi all, I intend to add address sanitizer (un)poison calls to Recycler and ArrayRecycler since I spent a few hours tracking down a bug in the AMDGPU backend that turned out to be a use-after-free that would have been detected by asan if it weren't for the Recycler. See https://reviews.llvm.org/D25313. Naturally, such a change exposes a bunch of bugs or things that are dodgy and happen not to be problematic today, but might easily break in the future, across all backends. I have prepared patches to fix all the issues in the CodeGen/AMDGPU lit tests; all the non-AMDGPU fixes are rolled into the patch above (although it probably makes sense to commit them first as a separate change before switching asan on). But it's clearly not my job to fix the inevitable build bot regressions in other backends. How do you want to handle this? Cheers, Nicolai
Mehdi Amini via llvm-dev
2016-Oct-06 22:31 UTC
[llvm-dev] Adding asan poison to Recycler and ArrayRecycler
> On Oct 6, 2016, at 4:54 AM, Nicolai Hähnle via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > Hi all, > > I intend to add address sanitizer (un)poison calls to Recycler and ArrayRecyclerThat’s a great thing to do! Looking forward for it.> since I spent a few hours tracking down a bug in the AMDGPU backend that turned out to be a use-after-free that would have been detected by asan if it weren't for the Recycler. See https://reviews.llvm.org/D25313. > > Naturally, such a change exposes a bunch of bugs or things that are dodgy and happen not to be problematic today, but might easily break in the future, across all backends. I have prepared patches to fix all the issues in the CodeGen/AMDGPU lit tests; all the non-AMDGPU fixes are rolled into the patch above (although it probably makes sense to commit them first as a separate change before switching asan on). But it's clearly not my job to fix the inevitable build bot regressions in other backends.Unfortunately, that does not sound as clear to me as it sounds to you apparently. If you want to change the Recycler/ArrayRecycler in a way that it is exposing bugs with ASAN in other places in LLVM, you’ll have first to fix these bugs. — Mehdi
Mehdi Amini via llvm-dev
2016-Oct-06 22:33 UTC
[llvm-dev] Adding asan poison to Recycler and ArrayRecycler
> On Oct 6, 2016, at 3:31 PM, Mehdi Amini via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > >> On Oct 6, 2016, at 4:54 AM, Nicolai Hähnle via llvm-dev <llvm-dev at lists.llvm.org> wrote: >> >> Hi all, >> >> I intend to add address sanitizer (un)poison calls to Recycler and ArrayRecycler > > That’s a great thing to do! Looking forward for it. > >> since I spent a few hours tracking down a bug in the AMDGPU backend that turned out to be a use-after-free that would have been detected by asan if it weren't for the Recycler. See https://reviews.llvm.org/D25313. >> >> Naturally, such a change exposes a bunch of bugs or things that are dodgy and happen not to be problematic today, but might easily break in the future, across all backends. I have prepared patches to fix all the issues in the CodeGen/AMDGPU lit tests; all the non-AMDGPU fixes are rolled into the patch above (although it probably makes sense to commit them first as a separate change before switching asan on). But it's clearly not my job to fix the inevitable build bot regressions in other backends. > > Unfortunately, that does not sound as clear to me as it sounds to you apparently. > > If you want to change the Recycler/ArrayRecycler in a way that it is exposing bugs with ASAN in other places in LLVM, you’ll have first to fix these bugs.Of course the alternative is to fill an entry in Bugzilla for each bug you find and wait for them to be fixed before you can land your patch. — Mehdi
Justin Bogner via llvm-dev
2016-Oct-06 22:49 UTC
[llvm-dev] Adding asan poison to Recycler and ArrayRecycler
Nicolai Hähnle via llvm-dev <llvm-dev at lists.llvm.org> writes:> Hi all, > > I intend to add address sanitizer (un)poison calls to Recycler and > ArrayRecycler since I spent a few hours tracking down a bug in the > AMDGPU backend that turned out to be a use-after-free that would have > been detected by asan if it weren't for the Recycler. See > https://reviews.llvm.org/D25313. > > Naturally, such a change exposes a bunch of bugs or things that are > dodgy and happen not to be problematic today, but might easily break > in the future, across all backends. I have prepared patches to fix all > the issues in the CodeGen/AMDGPU lit tests; all the non-AMDGPU fixes > are rolled into the patch above (although it probably makes sense to > commit them first as a separate change before switching asan on). But > it's clearly not my job to fix the inevitable build bot regressions in > other backends. > > How do you want to handle this?Changes like this are kind of tricky to stage, as you've noticed ;) You have a few options, ranging from least work to fastest: 1. File bugs for the other backends and attach the patch that adds the annotations. 2. Include the ASAN backtraces in those bugs (Presumably there are a lot of duplicates, so you can attach one backtrace per backend and possibly have to repeat, or spend some time trying to pick out the duplicates and file a bug each) 3. Fix everyone else's backends yourself Unfortunately, both (1) and (2) tend to take a depressingly long time, so it's kind of hard to actually do these things without resorting to (3). A lot of the failures look like they're in X86 and ARM though, and there are a lot of contributors to those, so (2) might not be too bad in this case.
Nicolai Hähnle via llvm-dev
2016-Oct-07 07:29 UTC
[llvm-dev] Adding asan poison to Recycler and ArrayRecycler
On 07.10.2016 00:31, Mehdi Amini wrote:>> On Oct 6, 2016, at 4:54 AM, Nicolai Hähnle via llvm-dev <llvm-dev at lists.llvm.org> wrote: >> >> Hi all, >> >> I intend to add address sanitizer (un)poison calls to Recycler and ArrayRecycler > > That’s a great thing to do! Looking forward for it. > >> since I spent a few hours tracking down a bug in the AMDGPU backend that turned out to be a use-after-free that would have been detected by asan if it weren't for the Recycler. See https://reviews.llvm.org/D25313. >> >> Naturally, such a change exposes a bunch of bugs or things that are dodgy and happen not to be problematic today, but might easily break in the future, across all backends. I have prepared patches to fix all the issues in the CodeGen/AMDGPU lit tests; all the non-AMDGPU fixes are rolled into the patch above (although it probably makes sense to commit them first as a separate change before switching asan on). But it's clearly not my job to fix the inevitable build bot regressions in other backends. > > Unfortunately, that does not sound as clear to me as it sounds to you apparently. > > If you want to change the Recycler/ArrayRecycler in a way that it is exposing bugs with ASAN in other places in LLVM, you’ll have first to fix these bugs.The point is that the bugs are already there, it's just that nobody notices them. I'm totally behind a rule that says not to introduce any regressions, but what I'm proposing doesn't do that. What I'm proposing is basically equivalent to introducing a buildbot with a previously untested configuration. Actually, that makes me think that there should be an XFAIL-based approach to this. Anyway, I'm going to go with Justin's (1) + (2) bug-filing suggestion first, and see how quick people are at fixing the issues. Cheers, Nicolai