- User Since
- Jun 28 2018, 11:39 AM (155 w, 6 h)
I don't see any messages in the reverts, but this change caused crashes both times it landed, in case that wasn't already the reason that it was reverted each time.
Tue, Jun 15
Mon, Jun 14
- Require tsan feature to avoid buildbot jobs that don't support -fsanitize=thread (asan, windows, 32bit, ARMv7/8)
Thanks! I'll wait for another CI notification to post before landing.
- Add requires for clang to avoid unknown gcc flag errors
The test I added is not the greatest test, but it catches the issue. Lemme know if this works.
- Add a codegen test
Thu, Jun 10
This commit seems to be causing an LLDB crash. I'm still working on gathering info and reducing it, but maybe the crash reason is obvious to you given this stack trace:
Tue, Jun 8
Mon, Jun 7
Thu, Jun 3
Thu, May 27
While building this, there's a warning about an unhandled switch:
It occurs on ArchiveWriter.cpp. I implement a way to write Big Archive. If you think it is better, I can add some code to remove warning, but my goal is to implement read and write for Big Archive, so it will be corrected in a future commit / PR.
Many people build with -Werror, so it would be good to handle the case even if it's explicitly ignored, like:
Wed, May 26
The issue I'm seeing seems more directly caused by SLP vectorization, as it goes away with -fno-slp-vectorize. This patch merely unblocks that bad optimization AFAICT.
FYI, I'm seeing what I think is a miscompile that bisects to this patch. Greatly simplified, the problematic snippet is this:
Thanks, this fixes the miscompile I'm seeing.
Tue, May 25
We're seeing some test failures that bisected to this patch, possibly a miscompile. The test failure is in the unit test for this file: https://github.com/google/tink/blob/master/cc/subtle/aes_eax_aesni.cc. Are there already any known issues with this patch?
May 13 2021
This patch introduces an assertion error we believe may be contributing to a miscompile (along with some other recent SLP patches -- this patch fixes the reduced case in http://llvm.org/PR50323, but doesn't fix the full case it was reduced from):
May 12 2021
Updated patch LG -- the unreduced test passes now (and has no assertion errors). Thanks! (Deferring to @nikic or others for actual re-review).
May 11 2021
Temporarily reverted in fec2945998947f04d672e9c5f33b57f7177474c0 to keep trunk clean.
Thanks for taking a look. Would it be all right to revert in the meantime? (I'm assuming the fix is not trivial)
May 7 2021
We're seeing some issues with this patch, potentially a miscompile. When we enable assertions we get an error about widening atomic loads -- I'm not sure this is the source of the miscompile we're seeing, but it certainly looks related (I think this is causing corruption and leading to issues elsewhere).
$ cat repro.ll ; ModuleID = 'repro.ll' source_filename = "repro.ll" target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-unknown-linux-gnu"
May 6 2021
I think we're all good now. The template depth fix in 84f0bb619507caf7c4809400138dae1da578282a actually exposed a long standing clang bug which needed another fix in 6bbfa0fd408e81055c360c2e059554dd76fd7f09. Thanks for the quick followups!
May 5 2021
Apr 30 2021
Another issue we're seeing is interactions between std::any and gmock -- I'll see if I can reduce it to something w/o the gmock dependency.
Apr 29 2021
We're seeing some issues with this patch manifested as complicated tuple expressions that hit compiler limits with template depth. A bit of a synthetic example, but here it is: https://godbolt.org/z/TzvzWhYYs
Apr 28 2021
Apr 27 2021
Landed 44e2247dcd04f3421164b085094eb575270564ba to fix LLDB. If you decide to go in a different direction again, please adjust that fix accordingly.
Apr 26 2021
Here's a C++ repro
I'm seeing a crash after the most recent time this was re-applied in 791930d74087b8ae8901172861a0fd21a211e436. The crash is when compiling https://github.com/beltoforion/muparser/blob/master/src/muParserTest.cpp, but I'm reducing it now, as the error might also depend on how we build that internally (we use at least`-g -O3`)
Apr 23 2021
LGTM too, thanks for writing this up!
Apr 22 2021
Added a few others that review binutils. Thanks for the patch!
Apr 21 2021
- Add comments about memory management for m_ipd_buf
Apr 20 2021
- Move comment about switch to the correct spot
Apr 19 2021
- Refactor cleanup of m_cxx_method_parser
Apr 15 2021
Since this is a shell (lit) test, I think you mean adding # REQUIRES: windows at the top of the test?
The motivation for this change is that sometimes engineers misidentify the output of these messages as the cause for a test failure
Apr 7 2021
Apr 5 2021
Mar 31 2021
Here's a repro for the issue we're seeing. It only seems to be happening with msan enabled, so I'm not sure if this is just an msan bug, or a general miscompile that *happens* to trigger in this test when msan is enabled.
We're still seeing some *very* strange miscompiles as a result of this patch (maybe not really this patch's fault, it could be some bad optimization that this unblocks). I'm working on a reduction right now.
Mar 29 2021
Thanks, the error message is much better now!
I don't have anything more to add now, so resigning now & leaving final thoughts and wording nits to other reviewers.
Mar 25 2021
As a whole, I think this patch is going in the right direction -- we definitely need more concrete guidance on reverting patches.
Mar 19 2021
I tried creating an IR repro, but: running -S -emit-llvm with the old PM crashes before it can generate anything, and running with the new PM seems to generate invalid IR (branches to %for.inc, which does not exist). I suspect this is not an optimizer bug, but crashing in the optimizer because invalid IR has been fed to it.
The error message I posted earlier was when using O1 + new PM, but it crashes with the old one & no optimizations:
We're seeing a crash in the optimizer after this patch, with the following logged in assert builds: assert.h assertion failed at llvm/include/llvm/Support/Casting.h:104 in static bool llvm::isa_impl_cl<llvm::InvokeInst, const llvm::Instruction *>::doit(const From *) [To = llvm::InvokeInst, From = const llvm::Instruction *]: Val && "isa<> used on a null pointer"
Mar 12 2021
Here's an IR reproducer, derived from the C++ one I provided in the reverting commit (as before, it reproduces with "clang -O2", although it doesn't with "opt -O2"):
Reverted in 8d20f2c2c66eb486ff23cc3d55a53bd840b36971. Although this fixes a crash, it also introduces a compile timeout (haven't checked if it's a true infinite loop or just _really_ slow) on other code. The reproducer is in the commit message.
Mar 9 2021
Feb 19 2021
This fixes a build error we're seeing, so I'd like to land this in a bit if that's OK.
Feb 12 2021
Feb 10 2021
Feb 5 2021
FWIW, I applied this patch and was able to make it pass this pretty exhaustive test:
We had 10-20 test failures internally that all bisected to this patch, and I logged the before/after of std::nth_element near the test failures. The values here are the relative ordering of the case that failed -- not the actual values, of course :)
Feb 4 2021
When I was reducing a test case, one thing I noticed was the problem only happens for certain pivot values, e.g.:
It looks like this is generating incorrect results. I uploaded D96074 which shows the failing test case.