Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
On 15 August 2017 at 08:56, Diana Picus via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Ugh, I'm not sure that last email made it through. > > Here's the IR with -Xclang -disable-llvm-passes, at r310400 and r310401: > https://goo.gl/4n84SR > https://goo.gl/gxhxp9 > > The diff between them is small enough so I don't think there's much > point in reducing it further. > Hope that helps. Yes, that helps a lot. The problem was that the implicitly-generated va_list struct for ARM wasn't getting the "CanPassInRegisters" flag set, and got passed indirectly (contrary to the ABI). I've added a test for that kind of problem in r310982, and re-committed the patch as r310983. Thanks for all your assistance here! > On 15 August 2017 at 17:51, Diana Picus wrote: > > Actually, I'm not sure I need to reduce it, the diff between the IR > > with -Xclang -no-llvm-passes is pretty small for the version > > with/without the patch. See attached. I hope this helps, it does look > > like a problem with va_arg handling. > > > > Sorry again that this is going so slowly... > > > > On 15 August 2017 at 16:52, Diana Picus wrote: > >> On 15 August 2017 at 01:25, Richard Smith > wrote: > >>> On 14 August 2017 at 03:27, Diana Picus via cfe-commits > >>> wrote: > > Hi, > > Strangely enough, it turns out that if I run > Asan-armhf-with-calls-Noinst-Test on the command line it fails, > although it doesn't fail when run with lit. > >>> > >>> > >>> Looks like the crash is within the "use_colors == true" portion of > >>> ColoredPrintf, so this would make sense if running the test within lit > turns > >>> off color support (perhaps because the output is not a terminal). > >>> > >> > >> That's a good catch, it seems if I run it with --gtest-color=no it > >> still passes, but prints some garbage at the end: > >> [ PASSED ] 97 tests. > >> YOU HAVE -1094542056 DISABLED F]�뀼��_8�pGD2 > >> > >> > > I've attached the stack > trace from gdb. It looks like some trouble passing down va_arg > parameters, but I haven't looked into too much details. The segfault > happens when we try to do a ldrb r3, [r0, r1], with r1 set to 0 by > the current function and r0 passed down from the caller. I'm not sure > if this is the exact same problem as the other tests, but feel free to > have a look at that code. > >>> > >>> > >>> Have you tried running Asan-armhf-with-calls-Noinst-Test on the > command line > >>> without this patch applied? (It's possible that this is a pre-existing > bug > >>> in ARM varargs call lowering, and is unrelated to the bug we're trying > to > >>> track down. This code path performs an unrelated varargs call between a > >>> va_start / va_end pair, which seems like quite a rare situation, and I > could > >>> easily believe there's something wrong with our lowering that allows > some > >>> portion of the outer va_list state to be clobbered in that scenario.) > >>> > >> > >> It seems to work without the patch. I'm currently trying to reduce > >> Asan-armhf-with-calls-Test and I'll get back to you with the IR or > >> assembly files (whichever makes any difference). > >> > Meanwhile, I've removed some clutter from Asan-armhf-with-calls-Test > (which is the original failure that we were seeing) and left only one > failing test that seemed small enough. I'll try to look at the > disassembly before/after the patch and maybe even run valgrind on it > (running it on the original binary naturally takes forever). > > Let me know if there's anything else I could try. I can also send you > disassembly or even LLVM IR for the Asan-armhf-with-calls-Noinst-Test > if you think it helps. > > Cheers, > Diana > ___ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
Ugh, I'm not sure that last email made it through. Here's the IR with -Xclang -disable-llvm-passes, at r310400 and r310401: https://goo.gl/4n84SR https://goo.gl/gxhxp9 The diff between them is small enough so I don't think there's much point in reducing it further. Hope that helps. On 15 August 2017 at 17:51, Diana Picus wrote: > Actually, I'm not sure I need to reduce it, the diff between the IR > with -Xclang -no-llvm-passes is pretty small for the version > with/without the patch. See attached. I hope this helps, it does look > like a problem with va_arg handling. > > Sorry again that this is going so slowly... > > On 15 August 2017 at 16:52, Diana Picus wrote: >> On 15 August 2017 at 01:25, Richard Smith wrote: >>> On 14 August 2017 at 03:27, Diana Picus via cfe-commits >>> wrote: Hi, Strangely enough, it turns out that if I run Asan-armhf-with-calls-Noinst-Test on the command line it fails, although it doesn't fail when run with lit. >>> >>> >>> Looks like the crash is within the "use_colors == true" portion of >>> ColoredPrintf, so this would make sense if running the test within lit turns >>> off color support (perhaps because the output is not a terminal). >>> >> >> That's a good catch, it seems if I run it with --gtest-color=no it >> still passes, but prints some garbage at the end: >> [ PASSED ] 97 tests. >> YOU HAVE -1094542056 DISABLED F]�뀼��_8�pGD2 >> >> I've attached the stack trace from gdb. It looks like some trouble passing down va_arg parameters, but I haven't looked into too much details. The segfault happens when we try to do a ldrb r3, [r0, r1], with r1 set to 0 by the current function and r0 passed down from the caller. I'm not sure if this is the exact same problem as the other tests, but feel free to have a look at that code. >>> >>> >>> Have you tried running Asan-armhf-with-calls-Noinst-Test on the command line >>> without this patch applied? (It's possible that this is a pre-existing bug >>> in ARM varargs call lowering, and is unrelated to the bug we're trying to >>> track down. This code path performs an unrelated varargs call between a >>> va_start / va_end pair, which seems like quite a rare situation, and I could >>> easily believe there's something wrong with our lowering that allows some >>> portion of the outer va_list state to be clobbered in that scenario.) >>> >> >> It seems to work without the patch. I'm currently trying to reduce >> Asan-armhf-with-calls-Test and I'll get back to you with the IR or >> assembly files (whichever makes any difference). >> Meanwhile, I've removed some clutter from Asan-armhf-with-calls-Test (which is the original failure that we were seeing) and left only one failing test that seemed small enough. I'll try to look at the disassembly before/after the patch and maybe even run valgrind on it (running it on the original binary naturally takes forever). Let me know if there's anything else I could try. I can also send you disassembly or even LLVM IR for the Asan-armhf-with-calls-Noinst-Test if you think it helps. Cheers, Diana ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
On 15 August 2017 at 01:25, Richard Smith wrote: > On 14 August 2017 at 03:27, Diana Picus via cfe-commits > wrote: >> >> Hi, >> >> Strangely enough, it turns out that if I run >> Asan-armhf-with-calls-Noinst-Test on the command line it fails, >> although it doesn't fail when run with lit. > > > Looks like the crash is within the "use_colors == true" portion of > ColoredPrintf, so this would make sense if running the test within lit turns > off color support (perhaps because the output is not a terminal). > That's a good catch, it seems if I run it with --gtest-color=no it still passes, but prints some garbage at the end: [ PASSED ] 97 tests. YOU HAVE -1094542056 DISABLED F]�뀼��_8�pGD2 >> >> I've attached the stack >> trace from gdb. It looks like some trouble passing down va_arg >> parameters, but I haven't looked into too much details. The segfault >> happens when we try to do a ldrb r3, [r0, r1], with r1 set to 0 by >> the current function and r0 passed down from the caller. I'm not sure >> if this is the exact same problem as the other tests, but feel free to >> have a look at that code. > > > Have you tried running Asan-armhf-with-calls-Noinst-Test on the command line > without this patch applied? (It's possible that this is a pre-existing bug > in ARM varargs call lowering, and is unrelated to the bug we're trying to > track down. This code path performs an unrelated varargs call between a > va_start / va_end pair, which seems like quite a rare situation, and I could > easily believe there's something wrong with our lowering that allows some > portion of the outer va_list state to be clobbered in that scenario.) > It seems to work without the patch. I'm currently trying to reduce Asan-armhf-with-calls-Test and I'll get back to you with the IR or assembly files (whichever makes any difference). >> Meanwhile, I've removed some clutter from Asan-armhf-with-calls-Test >> (which is the original failure that we were seeing) and left only one >> failing test that seemed small enough. I'll try to look at the >> disassembly before/after the patch and maybe even run valgrind on it >> (running it on the original binary naturally takes forever). >> >> Let me know if there's anything else I could try. I can also send you >> disassembly or even LLVM IR for the Asan-armhf-with-calls-Noinst-Test >> if you think it helps. >> >> Cheers, >> Diana ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
On 14 August 2017 at 03:27, Diana Picus via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Hi, > > Strangely enough, it turns out that if I run > Asan-armhf-with-calls-Noinst-Test on the command line it fails, > although it doesn't fail when run with lit. Looks like the crash is within the "use_colors == true" portion of ColoredPrintf, so this would make sense if running the test within lit turns off color support (perhaps because the output is not a terminal). > I've attached the stack > trace from gdb. It looks like some trouble passing down va_arg > parameters, but I haven't looked into too much details. The segfault > happens when we try to do a ldrb r3, [r0, r1], with r1 set to 0 by > the current function and r0 passed down from the caller. I'm not sure > if this is the exact same problem as the other tests, but feel free to > have a look at that code. > Have you tried running Asan-armhf-with-calls-Noinst-Test on the command line without this patch applied? (It's possible that this is a pre-existing bug in ARM varargs call lowering, and is unrelated to the bug we're trying to track down. This code path performs an unrelated varargs call between a va_start / va_end pair, which seems like quite a rare situation, and I could easily believe there's something wrong with our lowering that allows some portion of the outer va_list state to be clobbered in that scenario.) Meanwhile, I've removed some clutter from Asan-armhf-with-calls-Test > (which is the original failure that we were seeing) and left only one > failing test that seemed small enough. I'll try to look at the > disassembly before/after the patch and maybe even run valgrind on it > (running it on the original binary naturally takes forever). > > Let me know if there's anything else I could try. I can also send you > disassembly or even LLVM IR for the Asan-armhf-with-calls-Noinst-Test > if you think it helps. > > Cheers, > Diana ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
Hi, Ok that's a progress. Could you figure out which optimizer pass breaks it. I bet it is the inliner. I assume we could run a reducer on the ll file. Cheers, Vassil On 14/08/17 17:21, Diana Picus wrote: Hi, I didn't manage to reproduce this at -O0. Yes, I think the version in 1.8.0, slightly modified (see llvm/utils/unittest/googletest/README.LLVM) On 14 August 2017 at 17:06, Vassil Vassilev wrote: On 14/08/17 15:59, Diana Picus wrote: No, the buildbots don't build with -O0 (at least not the ones that broke). The command line for that particular object is: build/./bin/clang -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fcolor-diagnostics -ffunction-sections -fdata-sections -Wall -std=c++11 -Wno-unused-parameter -Wno-unknown-warning-option -Wno-covered-switch-default -DGTEST_NO_LLVM_RAW_OSTREAM=1 -DGTEST_HAS_RTTI=0 -I$LLVM_SRC/llvm/utils/unittest/googletest/include -I/home/diana.picus/linaro-scripts/bisect/llvm/utils/unittest/googletest -I$LLVM_SRC/llvm/projects/compiler-rt/include -I$LLVM_SRC/llvm/projects/compiler-rt/lib -I$LLVM_SRC/llvm/projects/compiler-rt/lib/asan -I$LLVM_SRC/llvm/projects/compiler-rt/lib/sanitizer_common/tests -fno-rtti -O2 -Wno-format -Werror=sign-compare -Wno-non-virtual-dtor -Wno-variadic-macros -gline-tables-only -DASAN_HAS_BLACKLIST=1 -DASAN_HAS_EXCEPTIONS=1 -DASAN_UAR=0 -fsanitize=address -fsanitize-blacklist=$LLVM_SRC/llvm/projects/compiler-rt/lib/asan/tests/asan_test.ignore -mllvm -asan-instrumentation-with-call-threshold=0 -march=armv7-a -mfloat-abi=hard -c -o ASAN_INST_TEST_OBJECTS.asan_test.cc.armhf-with-calls.o $LLVM_SRC/compiler-rt/lib/asan/tests/asan_test.cc Could you try to reproduce the issue with O0? This would rule out the optimizer. Could you resend the O0 ll file, maybe I could find something out. IIUC, the gtest version is 1.8.0? Why would it contain gtest.cc? It seems to contain gtest.h and a bunch of other internal gtest headers... On 14 August 2017 at 16:51, Vassil Vassilev wrote: On 14/08/17 15:08, Vassil Vassilev wrote: On 14/08/17 13:04, Diana Picus wrote: See attached. Thanks! It looks like asan_test.i doesn't have gtest.cc which appears in the stack trace. Am I missing something? Could you paste the compiler invocation. Are we building with -O0 (I see quite a lot of things inline). Could it be an optimizer problem? On 14 August 2017 at 13:30, Vassil Vassilev wrote: On 14/08/17 11:27, Diana Picus wrote: Hi, Strangely enough, it turns out that if I run Asan-armhf-with-calls-Noinst-Test on the command line it fails, although it doesn't fail when run with lit. I've attached the stack trace from gdb. It looks like some trouble passing down va_arg parameters, but I haven't looked into too much details. The segfault happens when we try to do a ldrb r3, [r0, r1], with r1 set to 0 by the current function and r0 passed down from the caller. I'm not sure if this is the exact same problem as the other tests, but feel free to have a look at that code. Meanwhile, I've removed some clutter from Asan-armhf-with-calls-Test (which is the original failure that we were seeing) and left only one failing test that seemed small enough. I'll try to look at the disassembly before/after the patch and maybe even run valgrind on it (running it on the original binary naturally takes forever). Let me know if there's anything else I could try. I can also send you disassembly or even LLVM IR for the Asan-armhf-with-calls-Noinst-Test if you think it helps. disassembly and LLVM will greatly help as well. Cheers, Diana On 11 August 2017 at 15:34, Diana Picus wrote: Well, these are ASAN tests, I'm not sure how that would interact with Valgrind. Anyway, I'll try to reproduce the environment, I'm guessing it would be best to catch this in gdb so I can actually see what's going on. On 11 August 2017 at 15:21, Vassil Vassilev wrote: That's really strange. It looks like some random behavior. Did you run some memory checker like valgrind? Do the environment provided by the test runner and yours match? Sent from my phone. Please excuse my brevity. On 11 Aug 2017, at 15:58, Diana Picus wrote: Hi again, I finally got the debug build, but unfortunately the stack traces that the tests print look the same. My suspicion is that this is because the addresses printed by the tests are funny (i.e. odd numbers instead of divisible by 4). I tried to follow those addresses in an objdump of the executable, but I didn't have much success since most of them weren't really pointing to call instructions. When I try to run the tests manually in the shell or in gdb, they pass. I'm not sure what else to try. Thoughts? Thanks, Diana On 11 August 2017 at 11:14, Diana Picus wrote: Hi guys, I'm SO sorry about the delays. I've been
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
Hi, I didn't manage to reproduce this at -O0. Yes, I think the version in 1.8.0, slightly modified (see llvm/utils/unittest/googletest/README.LLVM) On 14 August 2017 at 17:06, Vassil Vassilev wrote: > On 14/08/17 15:59, Diana Picus wrote: >> >> No, the buildbots don't build with -O0 (at least not the ones that broke). >> >> The command line for that particular object is: >> build/./bin/clang -fPIC -fvisibility-inlines-hidden -Werror=date-time >> -std=c++11 -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual >> -Wmissing-field-initializers -pedantic -Wno-long-long >> -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor >> -Wstring-conversion -fcolor-diagnostics -ffunction-sections >> -fdata-sections -Wall -std=c++11 -Wno-unused-parameter >> -Wno-unknown-warning-option -Wno-covered-switch-default >> -DGTEST_NO_LLVM_RAW_OSTREAM=1 -DGTEST_HAS_RTTI=0 >> -I$LLVM_SRC/llvm/utils/unittest/googletest/include >> -I/home/diana.picus/linaro-scripts/bisect/llvm/utils/unittest/googletest >> -I$LLVM_SRC/llvm/projects/compiler-rt/include >> -I$LLVM_SRC/llvm/projects/compiler-rt/lib >> -I$LLVM_SRC/llvm/projects/compiler-rt/lib/asan >> -I$LLVM_SRC/llvm/projects/compiler-rt/lib/sanitizer_common/tests >> -fno-rtti -O2 -Wno-format -Werror=sign-compare -Wno-non-virtual-dtor >> -Wno-variadic-macros -gline-tables-only -DASAN_HAS_BLACKLIST=1 >> -DASAN_HAS_EXCEPTIONS=1 -DASAN_UAR=0 -fsanitize=address >> >> -fsanitize-blacklist=$LLVM_SRC/llvm/projects/compiler-rt/lib/asan/tests/asan_test.ignore >> -mllvm -asan-instrumentation-with-call-threshold=0 -march=armv7-a >> -mfloat-abi=hard -c -o >> ASAN_INST_TEST_OBJECTS.asan_test.cc.armhf-with-calls.o >> $LLVM_SRC/compiler-rt/lib/asan/tests/asan_test.cc > > Could you try to reproduce the issue with O0? This would rule out the > optimizer. Could you resend the O0 ll file, maybe I could find something > out. IIUC, the gtest version is 1.8.0? > >> >> Why would it contain gtest.cc? It seems to contain gtest.h and a bunch >> of other internal gtest headers... >> >> >> On 14 August 2017 at 16:51, Vassil Vassilev >> wrote: >>> >>> On 14/08/17 15:08, Vassil Vassilev wrote: On 14/08/17 13:04, Diana Picus wrote: > > See attached. Thanks! It looks like asan_test.i doesn't have gtest.cc which appears in the stack trace. Am I missing something? >>> >>>Could you paste the compiler invocation. Are we building with -O0 (I >>> see >>> quite a lot of things inline). Could it be an optimizer problem? >>> > On 14 August 2017 at 13:30, Vassil Vassilev > wrote: >> >> On 14/08/17 11:27, Diana Picus wrote: >>> >>> Hi, >>> >>> Strangely enough, it turns out that if I run >>> Asan-armhf-with-calls-Noinst-Test on the command line it fails, >>> although it doesn't fail when run with lit. I've attached the stack >>> trace from gdb. It looks like some trouble passing down va_arg >>> parameters, but I haven't looked into too much details. The segfault >>> happens when we try to do a ldrb r3, [r0, r1], with r1 set to 0 >>> by >>> the current function and r0 passed down from the caller. I'm not sure >>> if this is the exact same problem as the other tests, but feel free >>> to >>> have a look at that code. >>> >>> Meanwhile, I've removed some clutter from Asan-armhf-with-calls-Test >>> (which is the original failure that we were seeing) and left only one >>> failing test that seemed small enough. I'll try to look at the >>> disassembly before/after the patch and maybe even run valgrind on it >>> (running it on the original binary naturally takes forever). >>> >>> Let me know if there's anything else I could try. I can also send you >>> disassembly or even LLVM IR for the Asan-armhf-with-calls-Noinst-Test >>> if you think it helps. >> >> disassembly and LLVM will greatly help as well. >> >>> Cheers, >>> Diana >>> >>> On 11 August 2017 at 15:34, Diana Picus >>> wrote: Well, these are ASAN tests, I'm not sure how that would interact with Valgrind. Anyway, I'll try to reproduce the environment, I'm guessing it would be best to catch this in gdb so I can actually see what's going on. On 11 August 2017 at 15:21, Vassil Vassilev wrote: > > That's really strange. It looks like some random behavior. Did you > run > some memory checker like valgrind? > > Do the environment provided by the test runner and yours match? > > Sent from my phone. Please excuse my brevity. > >> On 11 Aug 2017, at 15:58, Diana Picus >> wrote: >> >> Hi again, >> >> I finally got the debug build, but unfortunately the stack traces >> that >> the tests print look the same. My suspicion is that this is >> because
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
On 14/08/17 15:59, Diana Picus wrote: No, the buildbots don't build with -O0 (at least not the ones that broke). The command line for that particular object is: build/./bin/clang -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fcolor-diagnostics -ffunction-sections -fdata-sections -Wall -std=c++11 -Wno-unused-parameter -Wno-unknown-warning-option -Wno-covered-switch-default -DGTEST_NO_LLVM_RAW_OSTREAM=1 -DGTEST_HAS_RTTI=0 -I$LLVM_SRC/llvm/utils/unittest/googletest/include -I/home/diana.picus/linaro-scripts/bisect/llvm/utils/unittest/googletest -I$LLVM_SRC/llvm/projects/compiler-rt/include -I$LLVM_SRC/llvm/projects/compiler-rt/lib -I$LLVM_SRC/llvm/projects/compiler-rt/lib/asan -I$LLVM_SRC/llvm/projects/compiler-rt/lib/sanitizer_common/tests -fno-rtti -O2 -Wno-format -Werror=sign-compare -Wno-non-virtual-dtor -Wno-variadic-macros -gline-tables-only -DASAN_HAS_BLACKLIST=1 -DASAN_HAS_EXCEPTIONS=1 -DASAN_UAR=0 -fsanitize=address -fsanitize-blacklist=$LLVM_SRC/llvm/projects/compiler-rt/lib/asan/tests/asan_test.ignore -mllvm -asan-instrumentation-with-call-threshold=0 -march=armv7-a -mfloat-abi=hard -c -o ASAN_INST_TEST_OBJECTS.asan_test.cc.armhf-with-calls.o $LLVM_SRC/compiler-rt/lib/asan/tests/asan_test.cc Could you try to reproduce the issue with O0? This would rule out the optimizer. Could you resend the O0 ll file, maybe I could find something out. IIUC, the gtest version is 1.8.0? Why would it contain gtest.cc? It seems to contain gtest.h and a bunch of other internal gtest headers... On 14 August 2017 at 16:51, Vassil Vassilev wrote: On 14/08/17 15:08, Vassil Vassilev wrote: On 14/08/17 13:04, Diana Picus wrote: See attached. Thanks! It looks like asan_test.i doesn't have gtest.cc which appears in the stack trace. Am I missing something? Could you paste the compiler invocation. Are we building with -O0 (I see quite a lot of things inline). Could it be an optimizer problem? On 14 August 2017 at 13:30, Vassil Vassilev wrote: On 14/08/17 11:27, Diana Picus wrote: Hi, Strangely enough, it turns out that if I run Asan-armhf-with-calls-Noinst-Test on the command line it fails, although it doesn't fail when run with lit. I've attached the stack trace from gdb. It looks like some trouble passing down va_arg parameters, but I haven't looked into too much details. The segfault happens when we try to do a ldrb r3, [r0, r1], with r1 set to 0 by the current function and r0 passed down from the caller. I'm not sure if this is the exact same problem as the other tests, but feel free to have a look at that code. Meanwhile, I've removed some clutter from Asan-armhf-with-calls-Test (which is the original failure that we were seeing) and left only one failing test that seemed small enough. I'll try to look at the disassembly before/after the patch and maybe even run valgrind on it (running it on the original binary naturally takes forever). Let me know if there's anything else I could try. I can also send you disassembly or even LLVM IR for the Asan-armhf-with-calls-Noinst-Test if you think it helps. disassembly and LLVM will greatly help as well. Cheers, Diana On 11 August 2017 at 15:34, Diana Picus wrote: Well, these are ASAN tests, I'm not sure how that would interact with Valgrind. Anyway, I'll try to reproduce the environment, I'm guessing it would be best to catch this in gdb so I can actually see what's going on. On 11 August 2017 at 15:21, Vassil Vassilev wrote: That's really strange. It looks like some random behavior. Did you run some memory checker like valgrind? Do the environment provided by the test runner and yours match? Sent from my phone. Please excuse my brevity. On 11 Aug 2017, at 15:58, Diana Picus wrote: Hi again, I finally got the debug build, but unfortunately the stack traces that the tests print look the same. My suspicion is that this is because the addresses printed by the tests are funny (i.e. odd numbers instead of divisible by 4). I tried to follow those addresses in an objdump of the executable, but I didn't have much success since most of them weren't really pointing to call instructions. When I try to run the tests manually in the shell or in gdb, they pass. I'm not sure what else to try. Thoughts? Thanks, Diana On 11 August 2017 at 11:14, Diana Picus wrote: Hi guys, I'm SO sorry about the delays. I've been having all sorts of trouble getting that debug build on the board (from ld running out of memory to the board just crashing on me, in which case I need to ask someone else to reboot it because I can't power cycle it remotely). I can assure you this is one of my top priorities, I'll get those stack traces as soon as I can. Thanks for your patience and sorry again, Diana On 10 August 2017 at 22:55, Richard Smith w
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
On 14 August 2017 at 16:59, Diana Picus wrote: > No, the buildbots don't build with -O0 (at least not the ones that broke). > > The command line for that particular object is: > build/./bin/clang -fPIC -fvisibility-inlines-hidden -Werror=date-time > -std=c++11 -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual > -Wmissing-field-initializers -pedantic -Wno-long-long > -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor > -Wstring-conversion -fcolor-diagnostics -ffunction-sections > -fdata-sections -Wall -std=c++11 -Wno-unused-parameter > -Wno-unknown-warning-option -Wno-covered-switch-default > -DGTEST_NO_LLVM_RAW_OSTREAM=1 -DGTEST_HAS_RTTI=0 > -I$LLVM_SRC/llvm/utils/unittest/googletest/include > -I/home/diana.picus/linaro-scripts/bisect/llvm/utils/unittest/googletest > -I$LLVM_SRC/llvm/projects/compiler-rt/include > -I$LLVM_SRC/llvm/projects/compiler-rt/lib > -I$LLVM_SRC/llvm/projects/compiler-rt/lib/asan > -I$LLVM_SRC/llvm/projects/compiler-rt/lib/sanitizer_common/tests > -fno-rtti -O2 -Wno-format -Werror=sign-compare -Wno-non-virtual-dtor > -Wno-variadic-macros -gline-tables-only -DASAN_HAS_BLACKLIST=1 > -DASAN_HAS_EXCEPTIONS=1 -DASAN_UAR=0 -fsanitize=address > -fsanitize-blacklist=$LLVM_SRC/llvm/projects/compiler-rt/lib/asan/tests/asan_test.ignore > -mllvm -asan-instrumentation-with-call-threshold=0 -march=armv7-a > -mfloat-abi=hard -c -o > ASAN_INST_TEST_OBJECTS.asan_test.cc.armhf-with-calls.o > $LLVM_SRC/compiler-rt/lib/asan/tests/asan_test.cc > > Why would it contain gtest.cc? It seems to contain gtest.h and a bunch > of other internal gtest headers... > In particular, gtest.cc is included into gtest_all.cc, which compiled to ASAN_INST_TEST_OBJECTS.gtest_all.cc.armhf-with-calls.o and then added to the link line, which looks like this: build/./bin/clang ASAN_INST_TEST_OBJECTS.gtest-all.cc.armhf-with-calls.o ASAN_INST_TEST_OBJECTS.asan_asm_test.cc.armhf-with-calls.o ASAN_INST_TEST_OBJECTS.asan_globals_test.cc.armhf-with-calls.o ASAN_INST_TEST_OBJECTS.asan_interface_test.cc.armhf-with-calls.o ASAN_INST_TEST_OBJECTS.asan_internal_interface_test.cc.armhf-with-calls.o ASAN_INST_TEST_OBJECTS.asan_test.cc.armhf-with-calls.o ASAN_INST_TEST_OBJECTS.asan_oob_test.cc.armhf-with-calls.o ASAN_INST_TEST_OBJECTS.asan_mem_test.cc.armhf-with-calls.o ASAN_INST_TEST_OBJECTS.asan_str_test.cc.armhf-with-calls.o ASAN_INST_TEST_OBJECTS.asan_test_main.cc.armhf-with-calls.o -o build/projects/compiler-rt/lib/asan/tests/default/Asan-armhf-with-calls-Test -Wl,-allow-shlib-undefined -g --driver-mode=g++ -fsanitize=address -march=armv7-a -mfloat-abi=hard > > On 14 August 2017 at 16:51, Vassil Vassilev wrote: >> On 14/08/17 15:08, Vassil Vassilev wrote: >>> >>> On 14/08/17 13:04, Diana Picus wrote: See attached. >>> >>> Thanks! It looks like asan_test.i doesn't have gtest.cc which appears in >>> the stack trace. Am I missing something? >> >> Could you paste the compiler invocation. Are we building with -O0 (I see >> quite a lot of things inline). Could it be an optimizer problem? >> On 14 August 2017 at 13:30, Vassil Vassilev wrote: > > On 14/08/17 11:27, Diana Picus wrote: >> >> Hi, >> >> Strangely enough, it turns out that if I run >> Asan-armhf-with-calls-Noinst-Test on the command line it fails, >> although it doesn't fail when run with lit. I've attached the stack >> trace from gdb. It looks like some trouble passing down va_arg >> parameters, but I haven't looked into too much details. The segfault >> happens when we try to do a ldrb r3, [r0, r1], with r1 set to 0 by >> the current function and r0 passed down from the caller. I'm not sure >> if this is the exact same problem as the other tests, but feel free to >> have a look at that code. >> >> Meanwhile, I've removed some clutter from Asan-armhf-with-calls-Test >> (which is the original failure that we were seeing) and left only one >> failing test that seemed small enough. I'll try to look at the >> disassembly before/after the patch and maybe even run valgrind on it >> (running it on the original binary naturally takes forever). >> >> Let me know if there's anything else I could try. I can also send you >> disassembly or even LLVM IR for the Asan-armhf-with-calls-Noinst-Test >> if you think it helps. > >disassembly and LLVM will greatly help as well. > >> Cheers, >> Diana >> >> On 11 August 2017 at 15:34, Diana Picus wrote: >>> >>> Well, these are ASAN tests, I'm not sure how that would interact with >>> Valgrind. >>> Anyway, I'll try to reproduce the environment, I'm guessing it would >>> be best to catch this in gdb so I can actually see what's going on. >>> >>> On 11 August 2017 at 15:21, Vassil Vassilev >>> wrote: That's really strange. It looks like some random behavior. Did you run some memory checker like valg
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
No, the buildbots don't build with -O0 (at least not the ones that broke). The command line for that particular object is: build/./bin/clang -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fcolor-diagnostics -ffunction-sections -fdata-sections -Wall -std=c++11 -Wno-unused-parameter -Wno-unknown-warning-option -Wno-covered-switch-default -DGTEST_NO_LLVM_RAW_OSTREAM=1 -DGTEST_HAS_RTTI=0 -I$LLVM_SRC/llvm/utils/unittest/googletest/include -I/home/diana.picus/linaro-scripts/bisect/llvm/utils/unittest/googletest -I$LLVM_SRC/llvm/projects/compiler-rt/include -I$LLVM_SRC/llvm/projects/compiler-rt/lib -I$LLVM_SRC/llvm/projects/compiler-rt/lib/asan -I$LLVM_SRC/llvm/projects/compiler-rt/lib/sanitizer_common/tests -fno-rtti -O2 -Wno-format -Werror=sign-compare -Wno-non-virtual-dtor -Wno-variadic-macros -gline-tables-only -DASAN_HAS_BLACKLIST=1 -DASAN_HAS_EXCEPTIONS=1 -DASAN_UAR=0 -fsanitize=address -fsanitize-blacklist=$LLVM_SRC/llvm/projects/compiler-rt/lib/asan/tests/asan_test.ignore -mllvm -asan-instrumentation-with-call-threshold=0 -march=armv7-a -mfloat-abi=hard -c -o ASAN_INST_TEST_OBJECTS.asan_test.cc.armhf-with-calls.o $LLVM_SRC/compiler-rt/lib/asan/tests/asan_test.cc Why would it contain gtest.cc? It seems to contain gtest.h and a bunch of other internal gtest headers... On 14 August 2017 at 16:51, Vassil Vassilev wrote: > On 14/08/17 15:08, Vassil Vassilev wrote: >> >> On 14/08/17 13:04, Diana Picus wrote: >>> >>> See attached. >> >> Thanks! It looks like asan_test.i doesn't have gtest.cc which appears in >> the stack trace. Am I missing something? > > Could you paste the compiler invocation. Are we building with -O0 (I see > quite a lot of things inline). Could it be an optimizer problem? > >>> >>> On 14 August 2017 at 13:30, Vassil Vassilev >>> wrote: On 14/08/17 11:27, Diana Picus wrote: > > Hi, > > Strangely enough, it turns out that if I run > Asan-armhf-with-calls-Noinst-Test on the command line it fails, > although it doesn't fail when run with lit. I've attached the stack > trace from gdb. It looks like some trouble passing down va_arg > parameters, but I haven't looked into too much details. The segfault > happens when we try to do a ldrb r3, [r0, r1], with r1 set to 0 by > the current function and r0 passed down from the caller. I'm not sure > if this is the exact same problem as the other tests, but feel free to > have a look at that code. > > Meanwhile, I've removed some clutter from Asan-armhf-with-calls-Test > (which is the original failure that we were seeing) and left only one > failing test that seemed small enough. I'll try to look at the > disassembly before/after the patch and maybe even run valgrind on it > (running it on the original binary naturally takes forever). > > Let me know if there's anything else I could try. I can also send you > disassembly or even LLVM IR for the Asan-armhf-with-calls-Noinst-Test > if you think it helps. disassembly and LLVM will greatly help as well. > Cheers, > Diana > > On 11 August 2017 at 15:34, Diana Picus wrote: >> >> Well, these are ASAN tests, I'm not sure how that would interact with >> Valgrind. >> Anyway, I'll try to reproduce the environment, I'm guessing it would >> be best to catch this in gdb so I can actually see what's going on. >> >> On 11 August 2017 at 15:21, Vassil Vassilev >> wrote: >>> >>> That's really strange. It looks like some random behavior. Did you >>> run >>> some memory checker like valgrind? >>> >>> Do the environment provided by the test runner and yours match? >>> >>> Sent from my phone. Please excuse my brevity. >>> On 11 Aug 2017, at 15:58, Diana Picus wrote: Hi again, I finally got the debug build, but unfortunately the stack traces that the tests print look the same. My suspicion is that this is because the addresses printed by the tests are funny (i.e. odd numbers instead of divisible by 4). I tried to follow those addresses in an objdump of the executable, but I didn't have much success since most of them weren't really pointing to call instructions. When I try to run the tests manually in the shell or in gdb, they pass. I'm not sure what else to try. Thoughts? Thanks, Diana > On 11 August 2017 at 11:14, Diana Picus > wrote: > Hi guys, > > I'm SO sorry about the delays. I've been having all sorts of > trouble > getting that debug b
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
On 14/08/17 15:08, Vassil Vassilev wrote: On 14/08/17 13:04, Diana Picus wrote: See attached. Thanks! It looks like asan_test.i doesn't have gtest.cc which appears in the stack trace. Am I missing something? Could you paste the compiler invocation. Are we building with -O0 (I see quite a lot of things inline). Could it be an optimizer problem? On 14 August 2017 at 13:30, Vassil Vassilev wrote: On 14/08/17 11:27, Diana Picus wrote: Hi, Strangely enough, it turns out that if I run Asan-armhf-with-calls-Noinst-Test on the command line it fails, although it doesn't fail when run with lit. I've attached the stack trace from gdb. It looks like some trouble passing down va_arg parameters, but I haven't looked into too much details. The segfault happens when we try to do a ldrb r3, [r0, r1], with r1 set to 0 by the current function and r0 passed down from the caller. I'm not sure if this is the exact same problem as the other tests, but feel free to have a look at that code. Meanwhile, I've removed some clutter from Asan-armhf-with-calls-Test (which is the original failure that we were seeing) and left only one failing test that seemed small enough. I'll try to look at the disassembly before/after the patch and maybe even run valgrind on it (running it on the original binary naturally takes forever). Let me know if there's anything else I could try. I can also send you disassembly or even LLVM IR for the Asan-armhf-with-calls-Noinst-Test if you think it helps. disassembly and LLVM will greatly help as well. Cheers, Diana On 11 August 2017 at 15:34, Diana Picus wrote: Well, these are ASAN tests, I'm not sure how that would interact with Valgrind. Anyway, I'll try to reproduce the environment, I'm guessing it would be best to catch this in gdb so I can actually see what's going on. On 11 August 2017 at 15:21, Vassil Vassilev wrote: That's really strange. It looks like some random behavior. Did you run some memory checker like valgrind? Do the environment provided by the test runner and yours match? Sent from my phone. Please excuse my brevity. On 11 Aug 2017, at 15:58, Diana Picus wrote: Hi again, I finally got the debug build, but unfortunately the stack traces that the tests print look the same. My suspicion is that this is because the addresses printed by the tests are funny (i.e. odd numbers instead of divisible by 4). I tried to follow those addresses in an objdump of the executable, but I didn't have much success since most of them weren't really pointing to call instructions. When I try to run the tests manually in the shell or in gdb, they pass. I'm not sure what else to try. Thoughts? Thanks, Diana On 11 August 2017 at 11:14, Diana Picus wrote: Hi guys, I'm SO sorry about the delays. I've been having all sorts of trouble getting that debug build on the board (from ld running out of memory to the board just crashing on me, in which case I need to ask someone else to reboot it because I can't power cycle it remotely). I can assure you this is one of my top priorities, I'll get those stack traces as soon as I can. Thanks for your patience and sorry again, Diana On 10 August 2017 at 22:55, Richard Smith wrote: Any news on this? We want this change in Clang 5, so the sooner we can understand and fix this regression the better... On 10 August 2017 at 01:28, Diana Picus via cfe-commits wrote: Hi Vassil, My build is in progress, but since it's a full build it's probably going to take another couple of hours to complete. I'll let you know when it's done. Thanks, Diana On 10 August 2017 at 10:09, Vassil Vassilev wrote: It looks like I can not reproduce it on osx (non-arm)... :( On 09/08/17 22:54, Diana Picus wrote: Reverting this also fixed the selfhost bots: http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/2142 http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost/builds/2309 http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost-neon/builds/1819 I'm afraid the logs for those look even less helpful. On 9 August 2017 at 16:17, Diana Picus wrote: Hi, See attached. FWIW, when I ran this on a very similar machine, I got 194 failures, all of which went away after reverting. So there might be something fishy going on. Regards, Diana On 9 August 2017 at 15:02, Vassil Vassilev wrote: Hi Diana, It seems the service is down. Could you send us the details of the failures (incl stack traces if any) Many thanks, Vassil On 09/08/17 15:27, Diana Picus via cfe-commits wrote: Hi Richard, I'm sorry but I've reverted this in r310464 because it was breaking some ASAN tests on this bot: http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9452 Please let me know if I can help debug this. Cheers, Diana On 8 August 2017 at 21:14, Richard Smith via cfe-commits wrote: I forgot to say: Based on a patch by Vassil Vassilev, which was based on a patc
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
On 14/08/17 13:04, Diana Picus wrote: See attached. Thanks! It looks like asan_test.i doesn't have gtest.cc which appears in the stack trace. Am I missing something? On 14 August 2017 at 13:30, Vassil Vassilev wrote: On 14/08/17 11:27, Diana Picus wrote: Hi, Strangely enough, it turns out that if I run Asan-armhf-with-calls-Noinst-Test on the command line it fails, although it doesn't fail when run with lit. I've attached the stack trace from gdb. It looks like some trouble passing down va_arg parameters, but I haven't looked into too much details. The segfault happens when we try to do a ldrb r3, [r0, r1], with r1 set to 0 by the current function and r0 passed down from the caller. I'm not sure if this is the exact same problem as the other tests, but feel free to have a look at that code. Meanwhile, I've removed some clutter from Asan-armhf-with-calls-Test (which is the original failure that we were seeing) and left only one failing test that seemed small enough. I'll try to look at the disassembly before/after the patch and maybe even run valgrind on it (running it on the original binary naturally takes forever). Let me know if there's anything else I could try. I can also send you disassembly or even LLVM IR for the Asan-armhf-with-calls-Noinst-Test if you think it helps. disassembly and LLVM will greatly help as well. Cheers, Diana On 11 August 2017 at 15:34, Diana Picus wrote: Well, these are ASAN tests, I'm not sure how that would interact with Valgrind. Anyway, I'll try to reproduce the environment, I'm guessing it would be best to catch this in gdb so I can actually see what's going on. On 11 August 2017 at 15:21, Vassil Vassilev wrote: That's really strange. It looks like some random behavior. Did you run some memory checker like valgrind? Do the environment provided by the test runner and yours match? Sent from my phone. Please excuse my brevity. On 11 Aug 2017, at 15:58, Diana Picus wrote: Hi again, I finally got the debug build, but unfortunately the stack traces that the tests print look the same. My suspicion is that this is because the addresses printed by the tests are funny (i.e. odd numbers instead of divisible by 4). I tried to follow those addresses in an objdump of the executable, but I didn't have much success since most of them weren't really pointing to call instructions. When I try to run the tests manually in the shell or in gdb, they pass. I'm not sure what else to try. Thoughts? Thanks, Diana On 11 August 2017 at 11:14, Diana Picus wrote: Hi guys, I'm SO sorry about the delays. I've been having all sorts of trouble getting that debug build on the board (from ld running out of memory to the board just crashing on me, in which case I need to ask someone else to reboot it because I can't power cycle it remotely). I can assure you this is one of my top priorities, I'll get those stack traces as soon as I can. Thanks for your patience and sorry again, Diana On 10 August 2017 at 22:55, Richard Smith wrote: Any news on this? We want this change in Clang 5, so the sooner we can understand and fix this regression the better... On 10 August 2017 at 01:28, Diana Picus via cfe-commits wrote: Hi Vassil, My build is in progress, but since it's a full build it's probably going to take another couple of hours to complete. I'll let you know when it's done. Thanks, Diana On 10 August 2017 at 10:09, Vassil Vassilev wrote: It looks like I can not reproduce it on osx (non-arm)... :( On 09/08/17 22:54, Diana Picus wrote: Reverting this also fixed the selfhost bots: http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/2142 http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost/builds/2309 http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost-neon/builds/1819 I'm afraid the logs for those look even less helpful. On 9 August 2017 at 16:17, Diana Picus wrote: Hi, See attached. FWIW, when I ran this on a very similar machine, I got 194 failures, all of which went away after reverting. So there might be something fishy going on. Regards, Diana On 9 August 2017 at 15:02, Vassil Vassilev wrote: Hi Diana, It seems the service is down. Could you send us the details of the failures (incl stack traces if any) Many thanks, Vassil On 09/08/17 15:27, Diana Picus via cfe-commits wrote: Hi Richard, I'm sorry but I've reverted this in r310464 because it was breaking some ASAN tests on this bot: http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9452 Please let me know if I can help debug this. Cheers, Diana On 8 August 2017 at 21:14, Richard Smith via cfe-commits wrote: I forgot to say: Based on a patch by Vassil Vassilev, which was based on a patch by Bernd Schmidt, which was based on a patch by Reid Kleckner. On 8 August 2017 at 12:12, Richard Smith via cfe-commits wrote: Author: rsmith Date: Tue Aug 8 12:12:28 2017 New Revision: 310401 URL: http://llvm.org/view
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
Also if you want the disassembly for the whole test executable (with just that test in it): https://goo.gl/pjULbN It's 177MB though. On 14 August 2017 at 14:04, Diana Picus wrote: > See attached. > > On 14 August 2017 at 13:30, Vassil Vassilev wrote: >> On 14/08/17 11:27, Diana Picus wrote: >>> >>> Hi, >>> >>> Strangely enough, it turns out that if I run >>> Asan-armhf-with-calls-Noinst-Test on the command line it fails, >>> although it doesn't fail when run with lit. I've attached the stack >>> trace from gdb. It looks like some trouble passing down va_arg >>> parameters, but I haven't looked into too much details. The segfault >>> happens when we try to do a ldrb r3, [r0, r1], with r1 set to 0 by >>> the current function and r0 passed down from the caller. I'm not sure >>> if this is the exact same problem as the other tests, but feel free to >>> have a look at that code. >>> >>> Meanwhile, I've removed some clutter from Asan-armhf-with-calls-Test >>> (which is the original failure that we were seeing) and left only one >>> failing test that seemed small enough. I'll try to look at the >>> disassembly before/after the patch and maybe even run valgrind on it >>> (running it on the original binary naturally takes forever). >>> >>> Let me know if there's anything else I could try. I can also send you >>> disassembly or even LLVM IR for the Asan-armhf-with-calls-Noinst-Test >>> if you think it helps. >> >> disassembly and LLVM will greatly help as well. >> >>> >>> Cheers, >>> Diana >>> >>> On 11 August 2017 at 15:34, Diana Picus wrote: Well, these are ASAN tests, I'm not sure how that would interact with Valgrind. Anyway, I'll try to reproduce the environment, I'm guessing it would be best to catch this in gdb so I can actually see what's going on. On 11 August 2017 at 15:21, Vassil Vassilev wrote: > > That's really strange. It looks like some random behavior. Did you run > some memory checker like valgrind? > > Do the environment provided by the test runner and yours match? > > Sent from my phone. Please excuse my brevity. > >> On 11 Aug 2017, at 15:58, Diana Picus wrote: >> >> Hi again, >> >> I finally got the debug build, but unfortunately the stack traces that >> the tests print look the same. My suspicion is that this is because >> the addresses printed by the tests are funny (i.e. odd numbers instead >> of divisible by 4). I tried to follow those addresses in an objdump of >> the executable, but I didn't have much success since most of them >> weren't really pointing to call instructions. >> >> When I try to run the tests manually in the shell or in gdb, they pass. >> >> I'm not sure what else to try. Thoughts? >> >> Thanks, >> Diana >> >>> On 11 August 2017 at 11:14, Diana Picus >>> wrote: >>> Hi guys, >>> >>> I'm SO sorry about the delays. I've been having all sorts of trouble >>> getting that debug build on the board (from ld running out of memory >>> to the board just crashing on me, in which case I need to ask someone >>> else to reboot it because I can't power cycle it remotely). I can >>> assure you this is one of my top priorities, I'll get those stack >>> traces as soon as I can. >>> >>> Thanks for your patience and sorry again, >>> Diana >>> On 10 August 2017 at 22:55, Richard Smith wrote: Any news on this? We want this change in Clang 5, so the sooner we can understand and fix this regression the better... On 10 August 2017 at 01:28, Diana Picus via cfe-commits wrote: > > Hi Vassil, > > My build is in progress, but since it's a full build it's probably > going to take another couple of hours to complete. I'll let you know > when it's done. > > Thanks, > Diana > > On 10 August 2017 at 10:09, Vassil Vassilev > wrote: >> >> It looks like I can not reproduce it on osx (non-arm)... :( >>> >>> On 09/08/17 22:54, Diana Picus wrote: >>> >>> Reverting this also fixed the selfhost bots: >>> >>> >>> >>> http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/2142 >>> >>> >>> >>> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost/builds/2309 >>> >>> >>> >>> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost-neon/builds/1819 >>> >>> I'm afraid the logs for those look even less helpful. >>> On 9 August 2017 at 16:17, Diana Picus wrote: Hi, See attached. FWIW, when I ran this on a very similar machine, I got 194 fail
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
On 14/08/17 11:27, Diana Picus wrote: Hi, Strangely enough, it turns out that if I run Asan-armhf-with-calls-Noinst-Test on the command line it fails, although it doesn't fail when run with lit. I've attached the stack trace from gdb. It looks like some trouble passing down va_arg parameters, but I haven't looked into too much details. The segfault happens when we try to do a ldrb r3, [r0, r1], with r1 set to 0 by the current function and r0 passed down from the caller. I'm not sure if this is the exact same problem as the other tests, but feel free to have a look at that code. Meanwhile, I've removed some clutter from Asan-armhf-with-calls-Test (which is the original failure that we were seeing) and left only one failing test that seemed small enough. I'll try to look at the disassembly before/after the patch and maybe even run valgrind on it (running it on the original binary naturally takes forever). Let me know if there's anything else I could try. I can also send you disassembly or even LLVM IR for the Asan-armhf-with-calls-Noinst-Test if you think it helps. disassembly and LLVM will greatly help as well. Cheers, Diana On 11 August 2017 at 15:34, Diana Picus wrote: Well, these are ASAN tests, I'm not sure how that would interact with Valgrind. Anyway, I'll try to reproduce the environment, I'm guessing it would be best to catch this in gdb so I can actually see what's going on. On 11 August 2017 at 15:21, Vassil Vassilev wrote: That's really strange. It looks like some random behavior. Did you run some memory checker like valgrind? Do the environment provided by the test runner and yours match? Sent from my phone. Please excuse my brevity. On 11 Aug 2017, at 15:58, Diana Picus wrote: Hi again, I finally got the debug build, but unfortunately the stack traces that the tests print look the same. My suspicion is that this is because the addresses printed by the tests are funny (i.e. odd numbers instead of divisible by 4). I tried to follow those addresses in an objdump of the executable, but I didn't have much success since most of them weren't really pointing to call instructions. When I try to run the tests manually in the shell or in gdb, they pass. I'm not sure what else to try. Thoughts? Thanks, Diana On 11 August 2017 at 11:14, Diana Picus wrote: Hi guys, I'm SO sorry about the delays. I've been having all sorts of trouble getting that debug build on the board (from ld running out of memory to the board just crashing on me, in which case I need to ask someone else to reboot it because I can't power cycle it remotely). I can assure you this is one of my top priorities, I'll get those stack traces as soon as I can. Thanks for your patience and sorry again, Diana On 10 August 2017 at 22:55, Richard Smith wrote: Any news on this? We want this change in Clang 5, so the sooner we can understand and fix this regression the better... On 10 August 2017 at 01:28, Diana Picus via cfe-commits wrote: Hi Vassil, My build is in progress, but since it's a full build it's probably going to take another couple of hours to complete. I'll let you know when it's done. Thanks, Diana On 10 August 2017 at 10:09, Vassil Vassilev wrote: It looks like I can not reproduce it on osx (non-arm)... :( On 09/08/17 22:54, Diana Picus wrote: Reverting this also fixed the selfhost bots: http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/2142 http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost/builds/2309 http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost-neon/builds/1819 I'm afraid the logs for those look even less helpful. On 9 August 2017 at 16:17, Diana Picus wrote: Hi, See attached. FWIW, when I ran this on a very similar machine, I got 194 failures, all of which went away after reverting. So there might be something fishy going on. Regards, Diana On 9 August 2017 at 15:02, Vassil Vassilev wrote: Hi Diana, It seems the service is down. Could you send us the details of the failures (incl stack traces if any) Many thanks, Vassil On 09/08/17 15:27, Diana Picus via cfe-commits wrote: Hi Richard, I'm sorry but I've reverted this in r310464 because it was breaking some ASAN tests on this bot: http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9452 Please let me know if I can help debug this. Cheers, Diana On 8 August 2017 at 21:14, Richard Smith via cfe-commits wrote: I forgot to say: Based on a patch by Vassil Vassilev, which was based on a patch by Bernd Schmidt, which was based on a patch by Reid Kleckner. On 8 August 2017 at 12:12, Richard Smith via cfe-commits wrote: Author: rsmith Date: Tue Aug 8 12:12:28 2017 New Revision: 310401 URL: http://llvm.org/viewvc/llvm-project?rev=310401&view=rev Log: PR19668, PR23034: Fix handling of move constructors and deleted copy constructors when deciding whether classes should be passed indirectly. This fixes ABI differences between Clang and
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
Hi Diana, On 14/08/17 11:27, Diana Picus wrote: Hi, Strangely enough, it turns out that if I run Asan-armhf-with-calls-Noinst-Test on the command line it fails, although it doesn't fail when run with lit. I've attached the stack trace from gdb. It looks like some trouble passing down va_arg parameters, but I haven't looked into too much details. The segfault happens when we try to do a ldrb r3, [r0, r1], with r1 set to 0 by the current function and r0 passed down from the caller. I'm not sure if this is the exact same problem as the other tests, but feel free to have a look at that code. That smells like our patch is interfering in some way... Meanwhile, I've removed some clutter from Asan-armhf-with-calls-Test (which is the original failure that we were seeing) and left only one failing test that seemed small enough. I'll try to look at the disassembly before/after the patch and maybe even run valgrind on it (running it on the original binary naturally takes forever). Let me know if there's anything else I could try. I can also send you disassembly or even LLVM IR for the Asan-armhf-with-calls-Noinst-Test if you think it helps. Would you be able to run clang -E on the translation unit that crashes and send it over? If you could give us a minimal reproducer it would be even better. Cheers, Vassil Cheers, Diana On 11 August 2017 at 15:34, Diana Picus wrote: Well, these are ASAN tests, I'm not sure how that would interact with Valgrind. Anyway, I'll try to reproduce the environment, I'm guessing it would be best to catch this in gdb so I can actually see what's going on. On 11 August 2017 at 15:21, Vassil Vassilev wrote: That's really strange. It looks like some random behavior. Did you run some memory checker like valgrind? Do the environment provided by the test runner and yours match? Sent from my phone. Please excuse my brevity. On 11 Aug 2017, at 15:58, Diana Picus wrote: Hi again, I finally got the debug build, but unfortunately the stack traces that the tests print look the same. My suspicion is that this is because the addresses printed by the tests are funny (i.e. odd numbers instead of divisible by 4). I tried to follow those addresses in an objdump of the executable, but I didn't have much success since most of them weren't really pointing to call instructions. When I try to run the tests manually in the shell or in gdb, they pass. I'm not sure what else to try. Thoughts? Thanks, Diana On 11 August 2017 at 11:14, Diana Picus wrote: Hi guys, I'm SO sorry about the delays. I've been having all sorts of trouble getting that debug build on the board (from ld running out of memory to the board just crashing on me, in which case I need to ask someone else to reboot it because I can't power cycle it remotely). I can assure you this is one of my top priorities, I'll get those stack traces as soon as I can. Thanks for your patience and sorry again, Diana On 10 August 2017 at 22:55, Richard Smith wrote: Any news on this? We want this change in Clang 5, so the sooner we can understand and fix this regression the better... On 10 August 2017 at 01:28, Diana Picus via cfe-commits wrote: Hi Vassil, My build is in progress, but since it's a full build it's probably going to take another couple of hours to complete. I'll let you know when it's done. Thanks, Diana On 10 August 2017 at 10:09, Vassil Vassilev wrote: It looks like I can not reproduce it on osx (non-arm)... :( On 09/08/17 22:54, Diana Picus wrote: Reverting this also fixed the selfhost bots: http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/2142 http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost/builds/2309 http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost-neon/builds/1819 I'm afraid the logs for those look even less helpful. On 9 August 2017 at 16:17, Diana Picus wrote: Hi, See attached. FWIW, when I ran this on a very similar machine, I got 194 failures, all of which went away after reverting. So there might be something fishy going on. Regards, Diana On 9 August 2017 at 15:02, Vassil Vassilev wrote: Hi Diana, It seems the service is down. Could you send us the details of the failures (incl stack traces if any) Many thanks, Vassil On 09/08/17 15:27, Diana Picus via cfe-commits wrote: Hi Richard, I'm sorry but I've reverted this in r310464 because it was breaking some ASAN tests on this bot: http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9452 Please let me know if I can help debug this. Cheers, Diana On 8 August 2017 at 21:14, Richard Smith via cfe-commits wrote: I forgot to say: Based on a patch by Vassil Vassilev, which was based on a patch by Bernd Schmidt, which was based on a patch by Reid Kleckner. On 8 August 2017 at 12:12, Richard Smith via cfe-commits wrote: Author: rsmith Date: Tue Aug 8 12:12:28 2017 New Revision: 310401 URL: http://llvm.org/viewvc/llvm-project?rev=310401&vie
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
Hi, Strangely enough, it turns out that if I run Asan-armhf-with-calls-Noinst-Test on the command line it fails, although it doesn't fail when run with lit. I've attached the stack trace from gdb. It looks like some trouble passing down va_arg parameters, but I haven't looked into too much details. The segfault happens when we try to do a ldrb r3, [r0, r1], with r1 set to 0 by the current function and r0 passed down from the caller. I'm not sure if this is the exact same problem as the other tests, but feel free to have a look at that code. Meanwhile, I've removed some clutter from Asan-armhf-with-calls-Test (which is the original failure that we were seeing) and left only one failing test that seemed small enough. I'll try to look at the disassembly before/after the patch and maybe even run valgrind on it (running it on the original binary naturally takes forever). Let me know if there's anything else I could try. I can also send you disassembly or even LLVM IR for the Asan-armhf-with-calls-Noinst-Test if you think it helps. Cheers, Diana On 11 August 2017 at 15:34, Diana Picus wrote: > Well, these are ASAN tests, I'm not sure how that would interact with > Valgrind. > Anyway, I'll try to reproduce the environment, I'm guessing it would > be best to catch this in gdb so I can actually see what's going on. > > On 11 August 2017 at 15:21, Vassil Vassilev wrote: >> That's really strange. It looks like some random behavior. Did you run some >> memory checker like valgrind? >> >> Do the environment provided by the test runner and yours match? >> >> Sent from my phone. Please excuse my brevity. >> >>> On 11 Aug 2017, at 15:58, Diana Picus wrote: >>> >>> Hi again, >>> >>> I finally got the debug build, but unfortunately the stack traces that >>> the tests print look the same. My suspicion is that this is because >>> the addresses printed by the tests are funny (i.e. odd numbers instead >>> of divisible by 4). I tried to follow those addresses in an objdump of >>> the executable, but I didn't have much success since most of them >>> weren't really pointing to call instructions. >>> >>> When I try to run the tests manually in the shell or in gdb, they pass. >>> >>> I'm not sure what else to try. Thoughts? >>> >>> Thanks, >>> Diana >>> On 11 August 2017 at 11:14, Diana Picus wrote: Hi guys, I'm SO sorry about the delays. I've been having all sorts of trouble getting that debug build on the board (from ld running out of memory to the board just crashing on me, in which case I need to ask someone else to reboot it because I can't power cycle it remotely). I can assure you this is one of my top priorities, I'll get those stack traces as soon as I can. Thanks for your patience and sorry again, Diana > On 10 August 2017 at 22:55, Richard Smith wrote: > Any news on this? We want this change in Clang 5, so the sooner we can > understand and fix this regression the better... > > On 10 August 2017 at 01:28, Diana Picus via cfe-commits > wrote: >> >> Hi Vassil, >> >> My build is in progress, but since it's a full build it's probably >> going to take another couple of hours to complete. I'll let you know >> when it's done. >> >> Thanks, >> Diana >> >> On 10 August 2017 at 10:09, Vassil Vassilev >> wrote: >>> It looks like I can not reproduce it on osx (non-arm)... :( On 09/08/17 22:54, Diana Picus wrote: Reverting this also fixed the selfhost bots: http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/2142 http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost/builds/2309 http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost-neon/builds/1819 I'm afraid the logs for those look even less helpful. > On 9 August 2017 at 16:17, Diana Picus wrote: > > Hi, > > See attached. FWIW, when I ran this on a very similar machine, I got > 194 failures, all of which went away after reverting. So there might > be something fishy going on. > > Regards, > Diana > > On 9 August 2017 at 15:02, Vassil Vassilev > wrote: >> >> Hi Diana, >> >> It seems the service is down. Could you send us the details of the >> failures (incl stack traces if any) >> >> Many thanks, >> Vassil >> >>> On 09/08/17 15:27, Diana Picus via cfe-commits wrote: >>> >>> Hi Richard, >>> >>> I'm sorry but I've reverted this in r310464 because it was breaking >>> some ASAN tests on this bot: >>> >>> >>> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9452 >>> >>> Please
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
Well, these are ASAN tests, I'm not sure how that would interact with Valgrind. Anyway, I'll try to reproduce the environment, I'm guessing it would be best to catch this in gdb so I can actually see what's going on. On 11 August 2017 at 15:21, Vassil Vassilev wrote: > That's really strange. It looks like some random behavior. Did you run some > memory checker like valgrind? > > Do the environment provided by the test runner and yours match? > > Sent from my phone. Please excuse my brevity. > >> On 11 Aug 2017, at 15:58, Diana Picus wrote: >> >> Hi again, >> >> I finally got the debug build, but unfortunately the stack traces that >> the tests print look the same. My suspicion is that this is because >> the addresses printed by the tests are funny (i.e. odd numbers instead >> of divisible by 4). I tried to follow those addresses in an objdump of >> the executable, but I didn't have much success since most of them >> weren't really pointing to call instructions. >> >> When I try to run the tests manually in the shell or in gdb, they pass. >> >> I'm not sure what else to try. Thoughts? >> >> Thanks, >> Diana >> >>> On 11 August 2017 at 11:14, Diana Picus wrote: >>> Hi guys, >>> >>> I'm SO sorry about the delays. I've been having all sorts of trouble >>> getting that debug build on the board (from ld running out of memory >>> to the board just crashing on me, in which case I need to ask someone >>> else to reboot it because I can't power cycle it remotely). I can >>> assure you this is one of my top priorities, I'll get those stack >>> traces as soon as I can. >>> >>> Thanks for your patience and sorry again, >>> Diana >>> On 10 August 2017 at 22:55, Richard Smith wrote: Any news on this? We want this change in Clang 5, so the sooner we can understand and fix this regression the better... On 10 August 2017 at 01:28, Diana Picus via cfe-commits wrote: > > Hi Vassil, > > My build is in progress, but since it's a full build it's probably > going to take another couple of hours to complete. I'll let you know > when it's done. > > Thanks, > Diana > > On 10 August 2017 at 10:09, Vassil Vassilev > wrote: >> It looks like I can not reproduce it on osx (non-arm)... :( >>> On 09/08/17 22:54, Diana Picus wrote: >>> >>> Reverting this also fixed the selfhost bots: >>> >>> >>> http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/2142 >>> >>> >>> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost/builds/2309 >>> >>> >>> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost-neon/builds/1819 >>> >>> I'm afraid the logs for those look even less helpful. >>> On 9 August 2017 at 16:17, Diana Picus wrote: Hi, See attached. FWIW, when I ran this on a very similar machine, I got 194 failures, all of which went away after reverting. So there might be something fishy going on. Regards, Diana On 9 August 2017 at 15:02, Vassil Vassilev wrote: > > Hi Diana, > > It seems the service is down. Could you send us the details of the > failures (incl stack traces if any) > > Many thanks, > Vassil > >> On 09/08/17 15:27, Diana Picus via cfe-commits wrote: >> >> Hi Richard, >> >> I'm sorry but I've reverted this in r310464 because it was breaking >> some ASAN tests on this bot: >> >> >> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9452 >> >> Please let me know if I can help debug this. >> >> Cheers, >> Diana >> >> On 8 August 2017 at 21:14, Richard Smith via cfe-commits >> wrote: >>> >>> I forgot to say: >>> >>> Based on a patch by Vassil Vassilev, which was based on a patch by >>> Bernd >>> Schmidt, which was based on a patch by Reid Kleckner. >>> >>> On 8 August 2017 at 12:12, Richard Smith via cfe-commits >>> wrote: Author: rsmith Date: Tue Aug 8 12:12:28 2017 New Revision: 310401 URL: http://llvm.org/viewvc/llvm-project?rev=310401&view=rev Log: PR19668, PR23034: Fix handling of move constructors and deleted copy constructors when deciding whether classes should be passed indirectly. This fixes ABI differences between Clang and GCC: * Previously, Clang ignored the move constructor when making this determination. It now takes the move constructor into account, per htt
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
That's really strange. It looks like some random behavior. Did you run some memory checker like valgrind? Do the environment provided by the test runner and yours match? Sent from my phone. Please excuse my brevity. > On 11 Aug 2017, at 15:58, Diana Picus wrote: > > Hi again, > > I finally got the debug build, but unfortunately the stack traces that > the tests print look the same. My suspicion is that this is because > the addresses printed by the tests are funny (i.e. odd numbers instead > of divisible by 4). I tried to follow those addresses in an objdump of > the executable, but I didn't have much success since most of them > weren't really pointing to call instructions. > > When I try to run the tests manually in the shell or in gdb, they pass. > > I'm not sure what else to try. Thoughts? > > Thanks, > Diana > >> On 11 August 2017 at 11:14, Diana Picus wrote: >> Hi guys, >> >> I'm SO sorry about the delays. I've been having all sorts of trouble >> getting that debug build on the board (from ld running out of memory >> to the board just crashing on me, in which case I need to ask someone >> else to reboot it because I can't power cycle it remotely). I can >> assure you this is one of my top priorities, I'll get those stack >> traces as soon as I can. >> >> Thanks for your patience and sorry again, >> Diana >> >>> On 10 August 2017 at 22:55, Richard Smith wrote: >>> Any news on this? We want this change in Clang 5, so the sooner we can >>> understand and fix this regression the better... >>> >>> On 10 August 2017 at 01:28, Diana Picus via cfe-commits >>> wrote: Hi Vassil, My build is in progress, but since it's a full build it's probably going to take another couple of hours to complete. I'll let you know when it's done. Thanks, Diana On 10 August 2017 at 10:09, Vassil Vassilev wrote: > It looks like I can not reproduce it on osx (non-arm)... :( >> On 09/08/17 22:54, Diana Picus wrote: >> >> Reverting this also fixed the selfhost bots: >> >> >> http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/2142 >> >> >> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost/builds/2309 >> >> >> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost-neon/builds/1819 >> >> I'm afraid the logs for those look even less helpful. >> >>> On 9 August 2017 at 16:17, Diana Picus wrote: >>> >>> Hi, >>> >>> See attached. FWIW, when I ran this on a very similar machine, I got >>> 194 failures, all of which went away after reverting. So there might >>> be something fishy going on. >>> >>> Regards, >>> Diana >>> >>> On 9 August 2017 at 15:02, Vassil Vassilev >>> wrote: Hi Diana, It seems the service is down. Could you send us the details of the failures (incl stack traces if any) Many thanks, Vassil > On 09/08/17 15:27, Diana Picus via cfe-commits wrote: > > Hi Richard, > > I'm sorry but I've reverted this in r310464 because it was breaking > some ASAN tests on this bot: > > > http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9452 > > Please let me know if I can help debug this. > > Cheers, > Diana > > On 8 August 2017 at 21:14, Richard Smith via cfe-commits > wrote: >> >> I forgot to say: >> >> Based on a patch by Vassil Vassilev, which was based on a patch by >> Bernd >> Schmidt, which was based on a patch by Reid Kleckner. >> >> On 8 August 2017 at 12:12, Richard Smith via cfe-commits >> wrote: >>> >>> Author: rsmith >>> Date: Tue Aug 8 12:12:28 2017 >>> New Revision: 310401 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=310401&view=rev >>> Log: >>> PR19668, PR23034: Fix handling of move constructors and deleted >>> copy >>> constructors when deciding whether classes should be passed >>> indirectly. >>> >>> This fixes ABI differences between Clang and GCC: >>> >>> * Previously, Clang ignored the move constructor when making >>> this >>> determination. It now takes the move constructor into >>> account, >>> per >>> https://github.com/itanium-cxx-abi/cxx-abi/pull/17 (this >>> change >>> may >>> seem recent, but the ABI change was agreed on the Itanium C++ >>> ABI >>> list a long time ago). >>> >>> * Previously, Clang's behavior when the copy constructor was >>> deleted >>> was unstable -- dependi
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
Hi again, I finally got the debug build, but unfortunately the stack traces that the tests print look the same. My suspicion is that this is because the addresses printed by the tests are funny (i.e. odd numbers instead of divisible by 4). I tried to follow those addresses in an objdump of the executable, but I didn't have much success since most of them weren't really pointing to call instructions. When I try to run the tests manually in the shell or in gdb, they pass. I'm not sure what else to try. Thoughts? Thanks, Diana On 11 August 2017 at 11:14, Diana Picus wrote: > Hi guys, > > I'm SO sorry about the delays. I've been having all sorts of trouble > getting that debug build on the board (from ld running out of memory > to the board just crashing on me, in which case I need to ask someone > else to reboot it because I can't power cycle it remotely). I can > assure you this is one of my top priorities, I'll get those stack > traces as soon as I can. > > Thanks for your patience and sorry again, > Diana > > On 10 August 2017 at 22:55, Richard Smith wrote: >> Any news on this? We want this change in Clang 5, so the sooner we can >> understand and fix this regression the better... >> >> On 10 August 2017 at 01:28, Diana Picus via cfe-commits >> wrote: >>> >>> Hi Vassil, >>> >>> My build is in progress, but since it's a full build it's probably >>> going to take another couple of hours to complete. I'll let you know >>> when it's done. >>> >>> Thanks, >>> Diana >>> >>> On 10 August 2017 at 10:09, Vassil Vassilev >>> wrote: >>> > It looks like I can not reproduce it on osx (non-arm)... :( >>> > On 09/08/17 22:54, Diana Picus wrote: >>> >> >>> >> Reverting this also fixed the selfhost bots: >>> >> >>> >> >>> >> http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/2142 >>> >> >>> >> >>> >> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost/builds/2309 >>> >> >>> >> >>> >> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost-neon/builds/1819 >>> >> >>> >> I'm afraid the logs for those look even less helpful. >>> >> >>> >> On 9 August 2017 at 16:17, Diana Picus wrote: >>> >>> >>> >>> Hi, >>> >>> >>> >>> See attached. FWIW, when I ran this on a very similar machine, I got >>> >>> 194 failures, all of which went away after reverting. So there might >>> >>> be something fishy going on. >>> >>> >>> >>> Regards, >>> >>> Diana >>> >>> >>> >>> On 9 August 2017 at 15:02, Vassil Vassilev >>> >>> wrote: >>> >>> Hi Diana, >>> >>> It seems the service is down. Could you send us the details of the >>> failures (incl stack traces if any) >>> >>> Many thanks, >>> Vassil >>> >>> On 09/08/17 15:27, Diana Picus via cfe-commits wrote: >>> > >>> > Hi Richard, >>> > >>> > I'm sorry but I've reverted this in r310464 because it was breaking >>> > some ASAN tests on this bot: >>> > >>> > >>> > http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9452 >>> > >>> > Please let me know if I can help debug this. >>> > >>> > Cheers, >>> > Diana >>> > >>> > On 8 August 2017 at 21:14, Richard Smith via cfe-commits >>> > wrote: >>> >> >>> >> I forgot to say: >>> >> >>> >> Based on a patch by Vassil Vassilev, which was based on a patch by >>> >> Bernd >>> >> Schmidt, which was based on a patch by Reid Kleckner. >>> >> >>> >> On 8 August 2017 at 12:12, Richard Smith via cfe-commits >>> >> wrote: >>> >>> >>> >>> Author: rsmith >>> >>> Date: Tue Aug 8 12:12:28 2017 >>> >>> New Revision: 310401 >>> >>> >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=310401&view=rev >>> >>> Log: >>> >>> PR19668, PR23034: Fix handling of move constructors and deleted >>> >>> copy >>> >>> constructors when deciding whether classes should be passed >>> >>> indirectly. >>> >>> >>> >>> This fixes ABI differences between Clang and GCC: >>> >>> >>> >>>* Previously, Clang ignored the move constructor when making >>> >>> this >>> >>> determination. It now takes the move constructor into >>> >>> account, >>> >>> per >>> >>> https://github.com/itanium-cxx-abi/cxx-abi/pull/17 (this >>> >>> change >>> >>> may >>> >>> seem recent, but the ABI change was agreed on the Itanium C++ >>> >>> ABI >>> >>> list a long time ago). >>> >>> >>> >>>* Previously, Clang's behavior when the copy constructor was >>> >>> deleted >>> >>> was unstable -- depending on whether the lazy declaration of >>> >>> the >>> >>> copy constructor had been triggered, you might get different >>> >>> behavior. >>> >>> We now eagerly declare the copy constructor whenever its >>> >>> deletedness >>> >>> is unclear, and ignore deleted copy/move constructors when >>> >>> looking >>> >>> for >>>
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
Hi guys, I'm SO sorry about the delays. I've been having all sorts of trouble getting that debug build on the board (from ld running out of memory to the board just crashing on me, in which case I need to ask someone else to reboot it because I can't power cycle it remotely). I can assure you this is one of my top priorities, I'll get those stack traces as soon as I can. Thanks for your patience and sorry again, Diana On 10 August 2017 at 22:55, Richard Smith wrote: > Any news on this? We want this change in Clang 5, so the sooner we can > understand and fix this regression the better... > > On 10 August 2017 at 01:28, Diana Picus via cfe-commits > wrote: >> >> Hi Vassil, >> >> My build is in progress, but since it's a full build it's probably >> going to take another couple of hours to complete. I'll let you know >> when it's done. >> >> Thanks, >> Diana >> >> On 10 August 2017 at 10:09, Vassil Vassilev >> wrote: >> > It looks like I can not reproduce it on osx (non-arm)... :( >> > On 09/08/17 22:54, Diana Picus wrote: >> >> >> >> Reverting this also fixed the selfhost bots: >> >> >> >> >> >> http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/2142 >> >> >> >> >> >> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost/builds/2309 >> >> >> >> >> >> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost-neon/builds/1819 >> >> >> >> I'm afraid the logs for those look even less helpful. >> >> >> >> On 9 August 2017 at 16:17, Diana Picus wrote: >> >>> >> >>> Hi, >> >>> >> >>> See attached. FWIW, when I ran this on a very similar machine, I got >> >>> 194 failures, all of which went away after reverting. So there might >> >>> be something fishy going on. >> >>> >> >>> Regards, >> >>> Diana >> >>> >> >>> On 9 August 2017 at 15:02, Vassil Vassilev >> >>> wrote: >> >> Hi Diana, >> >> It seems the service is down. Could you send us the details of the >> failures (incl stack traces if any) >> >> Many thanks, >> Vassil >> >> On 09/08/17 15:27, Diana Picus via cfe-commits wrote: >> > >> > Hi Richard, >> > >> > I'm sorry but I've reverted this in r310464 because it was breaking >> > some ASAN tests on this bot: >> > >> > >> > http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9452 >> > >> > Please let me know if I can help debug this. >> > >> > Cheers, >> > Diana >> > >> > On 8 August 2017 at 21:14, Richard Smith via cfe-commits >> > wrote: >> >> >> >> I forgot to say: >> >> >> >> Based on a patch by Vassil Vassilev, which was based on a patch by >> >> Bernd >> >> Schmidt, which was based on a patch by Reid Kleckner. >> >> >> >> On 8 August 2017 at 12:12, Richard Smith via cfe-commits >> >> wrote: >> >>> >> >>> Author: rsmith >> >>> Date: Tue Aug 8 12:12:28 2017 >> >>> New Revision: 310401 >> >>> >> >>> URL: http://llvm.org/viewvc/llvm-project?rev=310401&view=rev >> >>> Log: >> >>> PR19668, PR23034: Fix handling of move constructors and deleted >> >>> copy >> >>> constructors when deciding whether classes should be passed >> >>> indirectly. >> >>> >> >>> This fixes ABI differences between Clang and GCC: >> >>> >> >>>* Previously, Clang ignored the move constructor when making >> >>> this >> >>> determination. It now takes the move constructor into >> >>> account, >> >>> per >> >>> https://github.com/itanium-cxx-abi/cxx-abi/pull/17 (this >> >>> change >> >>> may >> >>> seem recent, but the ABI change was agreed on the Itanium C++ >> >>> ABI >> >>> list a long time ago). >> >>> >> >>>* Previously, Clang's behavior when the copy constructor was >> >>> deleted >> >>> was unstable -- depending on whether the lazy declaration of >> >>> the >> >>> copy constructor had been triggered, you might get different >> >>> behavior. >> >>> We now eagerly declare the copy constructor whenever its >> >>> deletedness >> >>> is unclear, and ignore deleted copy/move constructors when >> >>> looking >> >>> for >> >>> a trivial such constructor. >> >>> >> >>> This also fixes an ABI difference between Clang and MSVC: >> >>> >> >>>* If the copy constructor would be implicitly deleted (but has >> >>> not >> >>> been >> >>> lazily declared yet), for instance because the class has an >> >>> rvalue >> >>> reference member, we would pass it directly. We now pass such >> >>> a >> >>> class >> >>> indirectly, matching MSVC. >> >>> >> >>> Modified: >> >>> cfe/trunk/include/clang/AST/DeclCXX.h >> >>> cfe/trunk/lib/AST/ASTImporter.cpp >> >>> cfe/trunk/lib/AST/DeclCXX.cpp >> >>> cfe/trunk/lib/CodeGen/CGCXXABI.cpp >>
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
Hi Diana,
Thanks for helping us out!
Cheers, Vassil
On 10/08/17 11:28, Diana Picus wrote:
Hi Vassil,
My build is in progress, but since it's a full build it's probably
going to take another couple of hours to complete. I'll let you know
when it's done.
Thanks,
Diana
On 10 August 2017 at 10:09, Vassil Vassilev wrote:
It looks like I can not reproduce it on osx (non-arm)... :(
On 09/08/17 22:54, Diana Picus wrote:
Reverting this also fixed the selfhost bots:
http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/2142
http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost/builds/2309
http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost-neon/builds/1819
I'm afraid the logs for those look even less helpful.
On 9 August 2017 at 16:17, Diana Picus wrote:
Hi,
See attached. FWIW, when I ran this on a very similar machine, I got
194 failures, all of which went away after reverting. So there might
be something fishy going on.
Regards,
Diana
On 9 August 2017 at 15:02, Vassil Vassilev
wrote:
Hi Diana,
It seems the service is down. Could you send us the details of the
failures (incl stack traces if any)
Many thanks,
Vassil
On 09/08/17 15:27, Diana Picus via cfe-commits wrote:
Hi Richard,
I'm sorry but I've reverted this in r310464 because it was breaking
some ASAN tests on this bot:
http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9452
Please let me know if I can help debug this.
Cheers,
Diana
On 8 August 2017 at 21:14, Richard Smith via cfe-commits
wrote:
I forgot to say:
Based on a patch by Vassil Vassilev, which was based on a patch by
Bernd
Schmidt, which was based on a patch by Reid Kleckner.
On 8 August 2017 at 12:12, Richard Smith via cfe-commits
wrote:
Author: rsmith
Date: Tue Aug 8 12:12:28 2017
New Revision: 310401
URL: http://llvm.org/viewvc/llvm-project?rev=310401&view=rev
Log:
PR19668, PR23034: Fix handling of move constructors and deleted copy
constructors when deciding whether classes should be passed
indirectly.
This fixes ABI differences between Clang and GCC:
* Previously, Clang ignored the move constructor when making this
determination. It now takes the move constructor into account,
per
https://github.com/itanium-cxx-abi/cxx-abi/pull/17 (this change
may
seem recent, but the ABI change was agreed on the Itanium C++
ABI
list a long time ago).
* Previously, Clang's behavior when the copy constructor was
deleted
was unstable -- depending on whether the lazy declaration of the
copy constructor had been triggered, you might get different
behavior.
We now eagerly declare the copy constructor whenever its
deletedness
is unclear, and ignore deleted copy/move constructors when
looking
for
a trivial such constructor.
This also fixes an ABI difference between Clang and MSVC:
* If the copy constructor would be implicitly deleted (but has not
been
lazily declared yet), for instance because the class has an
rvalue
reference member, we would pass it directly. We now pass such a
class
indirectly, matching MSVC.
Modified:
cfe/trunk/include/clang/AST/DeclCXX.h
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/lib/AST/DeclCXX.cpp
cfe/trunk/lib/CodeGen/CGCXXABI.cpp
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
cfe/trunk/lib/Sema/SemaDeclCXX.cpp
cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/test/CodeGenCXX/uncopyable-args.cpp
cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
Modified: cfe/trunk/include/clang/AST/DeclCXX.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=310401&r1=310400&r2=310401&view=diff
==
--- cfe/trunk/include/clang/AST/DeclCXX.h (original)
+++ cfe/trunk/include/clang/AST/DeclCXX.h Tue Aug 8 12:12:28 2017
@@ -375,6 +375,7 @@ class CXXRecordDecl : public RecordDecl
/// \brief These flags are \c true if a defaulted
corresponding
special
/// member can't be fully analyzed without performing overload
resolution.
/// @{
+unsigned NeedOverloadResolutionForCopyConstructor : 1;
unsigned NeedOverloadResolutionForMoveConstructor : 1;
unsigned NeedOverloadResolutionForMoveAssignment : 1;
unsigned NeedOverloadResolutionForDestructor : 1;
@@ -383,6 +384,7 @@ class CXXRecordDecl : public RecordDecl
/// \brief These flags are \c true if an implicit defaulted
corresponding
/// special member would be defined as deleted.
/// @{
+unsigned DefaultedCopyConstructorIsDeleted : 1;
unsigned DefaultedMoveConstructorIsDeleted : 1;
unsigned DefaultedMoveAssignmentIsDeleted : 1;
unsigned DefaultedDestructorIsDeleted : 1;
@@ -415,6 +417,12 @@ class CXXRe
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
Hi Vassil, My build is in progress, but since it's a full build it's probably going to take another couple of hours to complete. I'll let you know when it's done. Thanks, Diana On 10 August 2017 at 10:09, Vassil Vassilev wrote: > It looks like I can not reproduce it on osx (non-arm)... :( > On 09/08/17 22:54, Diana Picus wrote: >> >> Reverting this also fixed the selfhost bots: >> >> http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/2142 >> >> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost/builds/2309 >> >> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost-neon/builds/1819 >> >> I'm afraid the logs for those look even less helpful. >> >> On 9 August 2017 at 16:17, Diana Picus wrote: >>> >>> Hi, >>> >>> See attached. FWIW, when I ran this on a very similar machine, I got >>> 194 failures, all of which went away after reverting. So there might >>> be something fishy going on. >>> >>> Regards, >>> Diana >>> >>> On 9 August 2017 at 15:02, Vassil Vassilev >>> wrote: Hi Diana, It seems the service is down. Could you send us the details of the failures (incl stack traces if any) Many thanks, Vassil On 09/08/17 15:27, Diana Picus via cfe-commits wrote: > > Hi Richard, > > I'm sorry but I've reverted this in r310464 because it was breaking > some ASAN tests on this bot: > > http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9452 > > Please let me know if I can help debug this. > > Cheers, > Diana > > On 8 August 2017 at 21:14, Richard Smith via cfe-commits > wrote: >> >> I forgot to say: >> >> Based on a patch by Vassil Vassilev, which was based on a patch by >> Bernd >> Schmidt, which was based on a patch by Reid Kleckner. >> >> On 8 August 2017 at 12:12, Richard Smith via cfe-commits >> wrote: >>> >>> Author: rsmith >>> Date: Tue Aug 8 12:12:28 2017 >>> New Revision: 310401 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=310401&view=rev >>> Log: >>> PR19668, PR23034: Fix handling of move constructors and deleted copy >>> constructors when deciding whether classes should be passed >>> indirectly. >>> >>> This fixes ABI differences between Clang and GCC: >>> >>>* Previously, Clang ignored the move constructor when making this >>> determination. It now takes the move constructor into account, >>> per >>> https://github.com/itanium-cxx-abi/cxx-abi/pull/17 (this change >>> may >>> seem recent, but the ABI change was agreed on the Itanium C++ >>> ABI >>> list a long time ago). >>> >>>* Previously, Clang's behavior when the copy constructor was >>> deleted >>> was unstable -- depending on whether the lazy declaration of the >>> copy constructor had been triggered, you might get different >>> behavior. >>> We now eagerly declare the copy constructor whenever its >>> deletedness >>> is unclear, and ignore deleted copy/move constructors when >>> looking >>> for >>> a trivial such constructor. >>> >>> This also fixes an ABI difference between Clang and MSVC: >>> >>>* If the copy constructor would be implicitly deleted (but has not >>> been >>> lazily declared yet), for instance because the class has an >>> rvalue >>> reference member, we would pass it directly. We now pass such a >>> class >>> indirectly, matching MSVC. >>> >>> Modified: >>> cfe/trunk/include/clang/AST/DeclCXX.h >>> cfe/trunk/lib/AST/ASTImporter.cpp >>> cfe/trunk/lib/AST/DeclCXX.cpp >>> cfe/trunk/lib/CodeGen/CGCXXABI.cpp >>> cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp >>> cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp >>> cfe/trunk/lib/Sema/SemaDeclCXX.cpp >>> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp >>> cfe/trunk/lib/Serialization/ASTWriter.cpp >>> cfe/trunk/test/CodeGenCXX/uncopyable-args.cpp >>> cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp >>> >>> Modified: cfe/trunk/include/clang/AST/DeclCXX.h >>> URL: >>> >>> >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=310401&r1=310400&r2=310401&view=diff >>> >>> >>> >>> == >>> --- cfe/trunk/include/clang/AST/DeclCXX.h (original) >>> +++ cfe/trunk/include/clang/AST/DeclCXX.h Tue Aug 8 12:12:28 2017 >>> @@ -375,6 +375,7 @@ class CXXRecordDecl : public RecordDecl >>>/// \brief These flags are \c true if a defaulted >>> corresponding >>> special >>>/// member can't be fully analyzed without performing overload >>> resolution
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
It looks like I can not reproduce it on osx (non-arm)... :(
On 09/08/17 22:54, Diana Picus wrote:
Reverting this also fixed the selfhost bots:
http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/2142
http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost/builds/2309
http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost-neon/builds/1819
I'm afraid the logs for those look even less helpful.
On 9 August 2017 at 16:17, Diana Picus wrote:
Hi,
See attached. FWIW, when I ran this on a very similar machine, I got
194 failures, all of which went away after reverting. So there might
be something fishy going on.
Regards,
Diana
On 9 August 2017 at 15:02, Vassil Vassilev wrote:
Hi Diana,
It seems the service is down. Could you send us the details of the
failures (incl stack traces if any)
Many thanks,
Vassil
On 09/08/17 15:27, Diana Picus via cfe-commits wrote:
Hi Richard,
I'm sorry but I've reverted this in r310464 because it was breaking
some ASAN tests on this bot:
http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9452
Please let me know if I can help debug this.
Cheers,
Diana
On 8 August 2017 at 21:14, Richard Smith via cfe-commits
wrote:
I forgot to say:
Based on a patch by Vassil Vassilev, which was based on a patch by Bernd
Schmidt, which was based on a patch by Reid Kleckner.
On 8 August 2017 at 12:12, Richard Smith via cfe-commits
wrote:
Author: rsmith
Date: Tue Aug 8 12:12:28 2017
New Revision: 310401
URL: http://llvm.org/viewvc/llvm-project?rev=310401&view=rev
Log:
PR19668, PR23034: Fix handling of move constructors and deleted copy
constructors when deciding whether classes should be passed indirectly.
This fixes ABI differences between Clang and GCC:
* Previously, Clang ignored the move constructor when making this
determination. It now takes the move constructor into account, per
https://github.com/itanium-cxx-abi/cxx-abi/pull/17 (this change may
seem recent, but the ABI change was agreed on the Itanium C++ ABI
list a long time ago).
* Previously, Clang's behavior when the copy constructor was deleted
was unstable -- depending on whether the lazy declaration of the
copy constructor had been triggered, you might get different
behavior.
We now eagerly declare the copy constructor whenever its deletedness
is unclear, and ignore deleted copy/move constructors when looking
for
a trivial such constructor.
This also fixes an ABI difference between Clang and MSVC:
* If the copy constructor would be implicitly deleted (but has not
been
lazily declared yet), for instance because the class has an rvalue
reference member, we would pass it directly. We now pass such a
class
indirectly, matching MSVC.
Modified:
cfe/trunk/include/clang/AST/DeclCXX.h
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/lib/AST/DeclCXX.cpp
cfe/trunk/lib/CodeGen/CGCXXABI.cpp
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
cfe/trunk/lib/Sema/SemaDeclCXX.cpp
cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/test/CodeGenCXX/uncopyable-args.cpp
cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
Modified: cfe/trunk/include/clang/AST/DeclCXX.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=310401&r1=310400&r2=310401&view=diff
==
--- cfe/trunk/include/clang/AST/DeclCXX.h (original)
+++ cfe/trunk/include/clang/AST/DeclCXX.h Tue Aug 8 12:12:28 2017
@@ -375,6 +375,7 @@ class CXXRecordDecl : public RecordDecl
/// \brief These flags are \c true if a defaulted corresponding
special
/// member can't be fully analyzed without performing overload
resolution.
/// @{
+unsigned NeedOverloadResolutionForCopyConstructor : 1;
unsigned NeedOverloadResolutionForMoveConstructor : 1;
unsigned NeedOverloadResolutionForMoveAssignment : 1;
unsigned NeedOverloadResolutionForDestructor : 1;
@@ -383,6 +384,7 @@ class CXXRecordDecl : public RecordDecl
/// \brief These flags are \c true if an implicit defaulted
corresponding
/// special member would be defined as deleted.
/// @{
+unsigned DefaultedCopyConstructorIsDeleted : 1;
unsigned DefaultedMoveConstructorIsDeleted : 1;
unsigned DefaultedMoveAssignmentIsDeleted : 1;
unsigned DefaultedDestructorIsDeleted : 1;
@@ -415,6 +417,12 @@ class CXXRecordDecl : public RecordDecl
/// constructor.
unsigned HasDefaultedDefaultConstructor : 1;
+/// \brief True if this class can be passed in a
non-address-preserving
+/// fashion (such as in registers) according to the C++ language
rules.
+/// This does not imply anything about how the ABI in use will
actually
+/// pass an object of this clas
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
On 09/08/17 22:54, Diana Picus wrote:
Reverting this also fixed the selfhost bots:
http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/2142
http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost/builds/2309
http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost-neon/builds/1819
I'm afraid the logs for those look even less helpful.
Not helpful indeed :) Do you have access to the machine and would you be
willing to build in debug mode and send us some stack traces. I do not
have any arm machines :(
On 9 August 2017 at 16:17, Diana Picus wrote:
Hi,
See attached. FWIW, when I ran this on a very similar machine, I got
194 failures, all of which went away after reverting. So there might
be something fishy going on.
Regards,
Diana
On 9 August 2017 at 15:02, Vassil Vassilev wrote:
Hi Diana,
It seems the service is down. Could you send us the details of the
failures (incl stack traces if any)
Many thanks,
Vassil
On 09/08/17 15:27, Diana Picus via cfe-commits wrote:
Hi Richard,
I'm sorry but I've reverted this in r310464 because it was breaking
some ASAN tests on this bot:
http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9452
Please let me know if I can help debug this.
Cheers,
Diana
On 8 August 2017 at 21:14, Richard Smith via cfe-commits
wrote:
I forgot to say:
Based on a patch by Vassil Vassilev, which was based on a patch by Bernd
Schmidt, which was based on a patch by Reid Kleckner.
On 8 August 2017 at 12:12, Richard Smith via cfe-commits
wrote:
Author: rsmith
Date: Tue Aug 8 12:12:28 2017
New Revision: 310401
URL: http://llvm.org/viewvc/llvm-project?rev=310401&view=rev
Log:
PR19668, PR23034: Fix handling of move constructors and deleted copy
constructors when deciding whether classes should be passed indirectly.
This fixes ABI differences between Clang and GCC:
* Previously, Clang ignored the move constructor when making this
determination. It now takes the move constructor into account, per
https://github.com/itanium-cxx-abi/cxx-abi/pull/17 (this change may
seem recent, but the ABI change was agreed on the Itanium C++ ABI
list a long time ago).
* Previously, Clang's behavior when the copy constructor was deleted
was unstable -- depending on whether the lazy declaration of the
copy constructor had been triggered, you might get different
behavior.
We now eagerly declare the copy constructor whenever its deletedness
is unclear, and ignore deleted copy/move constructors when looking
for
a trivial such constructor.
This also fixes an ABI difference between Clang and MSVC:
* If the copy constructor would be implicitly deleted (but has not
been
lazily declared yet), for instance because the class has an rvalue
reference member, we would pass it directly. We now pass such a
class
indirectly, matching MSVC.
Modified:
cfe/trunk/include/clang/AST/DeclCXX.h
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/lib/AST/DeclCXX.cpp
cfe/trunk/lib/CodeGen/CGCXXABI.cpp
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
cfe/trunk/lib/Sema/SemaDeclCXX.cpp
cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/test/CodeGenCXX/uncopyable-args.cpp
cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
Modified: cfe/trunk/include/clang/AST/DeclCXX.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=310401&r1=310400&r2=310401&view=diff
==
--- cfe/trunk/include/clang/AST/DeclCXX.h (original)
+++ cfe/trunk/include/clang/AST/DeclCXX.h Tue Aug 8 12:12:28 2017
@@ -375,6 +375,7 @@ class CXXRecordDecl : public RecordDecl
/// \brief These flags are \c true if a defaulted corresponding
special
/// member can't be fully analyzed without performing overload
resolution.
/// @{
+unsigned NeedOverloadResolutionForCopyConstructor : 1;
unsigned NeedOverloadResolutionForMoveConstructor : 1;
unsigned NeedOverloadResolutionForMoveAssignment : 1;
unsigned NeedOverloadResolutionForDestructor : 1;
@@ -383,6 +384,7 @@ class CXXRecordDecl : public RecordDecl
/// \brief These flags are \c true if an implicit defaulted
corresponding
/// special member would be defined as deleted.
/// @{
+unsigned DefaultedCopyConstructorIsDeleted : 1;
unsigned DefaultedMoveConstructorIsDeleted : 1;
unsigned DefaultedMoveAssignmentIsDeleted : 1;
unsigned DefaultedDestructorIsDeleted : 1;
@@ -415,6 +417,12 @@ class CXXRecordDecl : public RecordDecl
/// constructor.
unsigned HasDefaultedDefaultConstructor : 1;
+/// \brief True if this class can be passed in a
non-address-preserving
+/// fashion (such as in registers) according to the C++ language
rules.
+
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
Reverting this also fixed the selfhost bots:
http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/2142
http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost/builds/2309
http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-selfhost-neon/builds/1819
I'm afraid the logs for those look even less helpful.
On 9 August 2017 at 16:17, Diana Picus wrote:
> Hi,
>
> See attached. FWIW, when I ran this on a very similar machine, I got
> 194 failures, all of which went away after reverting. So there might
> be something fishy going on.
>
> Regards,
> Diana
>
> On 9 August 2017 at 15:02, Vassil Vassilev wrote:
>> Hi Diana,
>>
>> It seems the service is down. Could you send us the details of the
>> failures (incl stack traces if any)
>>
>> Many thanks,
>> Vassil
>>
>> On 09/08/17 15:27, Diana Picus via cfe-commits wrote:
>>>
>>> Hi Richard,
>>>
>>> I'm sorry but I've reverted this in r310464 because it was breaking
>>> some ASAN tests on this bot:
>>> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9452
>>>
>>> Please let me know if I can help debug this.
>>>
>>> Cheers,
>>> Diana
>>>
>>> On 8 August 2017 at 21:14, Richard Smith via cfe-commits
>>> wrote:
I forgot to say:
Based on a patch by Vassil Vassilev, which was based on a patch by Bernd
Schmidt, which was based on a patch by Reid Kleckner.
On 8 August 2017 at 12:12, Richard Smith via cfe-commits
wrote:
>
> Author: rsmith
> Date: Tue Aug 8 12:12:28 2017
> New Revision: 310401
>
> URL: http://llvm.org/viewvc/llvm-project?rev=310401&view=rev
> Log:
> PR19668, PR23034: Fix handling of move constructors and deleted copy
> constructors when deciding whether classes should be passed indirectly.
>
> This fixes ABI differences between Clang and GCC:
>
> * Previously, Clang ignored the move constructor when making this
> determination. It now takes the move constructor into account, per
> https://github.com/itanium-cxx-abi/cxx-abi/pull/17 (this change may
> seem recent, but the ABI change was agreed on the Itanium C++ ABI
> list a long time ago).
>
> * Previously, Clang's behavior when the copy constructor was deleted
> was unstable -- depending on whether the lazy declaration of the
> copy constructor had been triggered, you might get different
> behavior.
> We now eagerly declare the copy constructor whenever its deletedness
> is unclear, and ignore deleted copy/move constructors when looking
> for
> a trivial such constructor.
>
> This also fixes an ABI difference between Clang and MSVC:
>
> * If the copy constructor would be implicitly deleted (but has not
> been
> lazily declared yet), for instance because the class has an rvalue
> reference member, we would pass it directly. We now pass such a
> class
> indirectly, matching MSVC.
>
> Modified:
> cfe/trunk/include/clang/AST/DeclCXX.h
> cfe/trunk/lib/AST/ASTImporter.cpp
> cfe/trunk/lib/AST/DeclCXX.cpp
> cfe/trunk/lib/CodeGen/CGCXXABI.cpp
> cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
> cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
> cfe/trunk/lib/Sema/SemaDeclCXX.cpp
> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
> cfe/trunk/lib/Serialization/ASTWriter.cpp
> cfe/trunk/test/CodeGenCXX/uncopyable-args.cpp
> cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
>
> Modified: cfe/trunk/include/clang/AST/DeclCXX.h
> URL:
>
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=310401&r1=310400&r2=310401&view=diff
>
>
> ==
> --- cfe/trunk/include/clang/AST/DeclCXX.h (original)
> +++ cfe/trunk/include/clang/AST/DeclCXX.h Tue Aug 8 12:12:28 2017
> @@ -375,6 +375,7 @@ class CXXRecordDecl : public RecordDecl
> /// \brief These flags are \c true if a defaulted corresponding
> special
> /// member can't be fully analyzed without performing overload
> resolution.
> /// @{
> +unsigned NeedOverloadResolutionForCopyConstructor : 1;
> unsigned NeedOverloadResolutionForMoveConstructor : 1;
> unsigned NeedOverloadResolutionForMoveAssignment : 1;
> unsigned NeedOverloadResolutionForDestructor : 1;
> @@ -383,6 +384,7 @@ class CXXRecordDecl : public RecordDecl
> /// \brief These flags are \c true if an implicit defaulted
> corresponding
> /// special member would be defined as deleted.
> /// @{
> +unsigned DefaultedCopyConstructorIsDeleted : 1;
> unsigned DefaultedMoveConstructorIsDeleted : 1;
> unsigned DefaultedMoveAssignmentIsDeleted : 1;
> unsigned D
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
Hi Diana,
It seems the service is down. Could you send us the details of the
failures (incl stack traces if any)
Many thanks,
Vassil
On 09/08/17 15:27, Diana Picus via cfe-commits wrote:
Hi Richard,
I'm sorry but I've reverted this in r310464 because it was breaking
some ASAN tests on this bot:
http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9452
Please let me know if I can help debug this.
Cheers,
Diana
On 8 August 2017 at 21:14, Richard Smith via cfe-commits
wrote:
I forgot to say:
Based on a patch by Vassil Vassilev, which was based on a patch by Bernd
Schmidt, which was based on a patch by Reid Kleckner.
On 8 August 2017 at 12:12, Richard Smith via cfe-commits
wrote:
Author: rsmith
Date: Tue Aug 8 12:12:28 2017
New Revision: 310401
URL: http://llvm.org/viewvc/llvm-project?rev=310401&view=rev
Log:
PR19668, PR23034: Fix handling of move constructors and deleted copy
constructors when deciding whether classes should be passed indirectly.
This fixes ABI differences between Clang and GCC:
* Previously, Clang ignored the move constructor when making this
determination. It now takes the move constructor into account, per
https://github.com/itanium-cxx-abi/cxx-abi/pull/17 (this change may
seem recent, but the ABI change was agreed on the Itanium C++ ABI
list a long time ago).
* Previously, Clang's behavior when the copy constructor was deleted
was unstable -- depending on whether the lazy declaration of the
copy constructor had been triggered, you might get different behavior.
We now eagerly declare the copy constructor whenever its deletedness
is unclear, and ignore deleted copy/move constructors when looking for
a trivial such constructor.
This also fixes an ABI difference between Clang and MSVC:
* If the copy constructor would be implicitly deleted (but has not been
lazily declared yet), for instance because the class has an rvalue
reference member, we would pass it directly. We now pass such a class
indirectly, matching MSVC.
Modified:
cfe/trunk/include/clang/AST/DeclCXX.h
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/lib/AST/DeclCXX.cpp
cfe/trunk/lib/CodeGen/CGCXXABI.cpp
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
cfe/trunk/lib/Sema/SemaDeclCXX.cpp
cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/test/CodeGenCXX/uncopyable-args.cpp
cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
Modified: cfe/trunk/include/clang/AST/DeclCXX.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=310401&r1=310400&r2=310401&view=diff
==
--- cfe/trunk/include/clang/AST/DeclCXX.h (original)
+++ cfe/trunk/include/clang/AST/DeclCXX.h Tue Aug 8 12:12:28 2017
@@ -375,6 +375,7 @@ class CXXRecordDecl : public RecordDecl
/// \brief These flags are \c true if a defaulted corresponding
special
/// member can't be fully analyzed without performing overload
resolution.
/// @{
+unsigned NeedOverloadResolutionForCopyConstructor : 1;
unsigned NeedOverloadResolutionForMoveConstructor : 1;
unsigned NeedOverloadResolutionForMoveAssignment : 1;
unsigned NeedOverloadResolutionForDestructor : 1;
@@ -383,6 +384,7 @@ class CXXRecordDecl : public RecordDecl
/// \brief These flags are \c true if an implicit defaulted
corresponding
/// special member would be defined as deleted.
/// @{
+unsigned DefaultedCopyConstructorIsDeleted : 1;
unsigned DefaultedMoveConstructorIsDeleted : 1;
unsigned DefaultedMoveAssignmentIsDeleted : 1;
unsigned DefaultedDestructorIsDeleted : 1;
@@ -415,6 +417,12 @@ class CXXRecordDecl : public RecordDecl
/// constructor.
unsigned HasDefaultedDefaultConstructor : 1;
+/// \brief True if this class can be passed in a
non-address-preserving
+/// fashion (such as in registers) according to the C++ language
rules.
+/// This does not imply anything about how the ABI in use will
actually
+/// pass an object of this class.
+unsigned CanPassInRegisters : 1;
+
/// \brief True if a defaulted default constructor for this class
would
/// be constexpr.
unsigned DefaultedDefaultConstructorIsConstexpr : 1;
@@ -811,18 +819,50 @@ public:
return data().FirstFriend.isValid();
}
+ /// \brief \c true if a defaulted copy constructor for this class would
be
+ /// deleted.
+ bool defaultedCopyConstructorIsDeleted() const {
+assert((!needsOverloadResolutionForCopyConstructor() ||
+(data().DeclaredSpecialMembers & SMF_CopyConstructor)) &&
+ "this property has not yet been computed by Sema");
+return data().DefaultedCopyConstructorIsDeleted;
+ }
+
+ /// \brief \c true if a defaulted move constructor for this class would
be
+
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
Hi Richard,
I'm sorry but I've reverted this in r310464 because it was breaking
some ASAN tests on this bot:
http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9452
Please let me know if I can help debug this.
Cheers,
Diana
On 8 August 2017 at 21:14, Richard Smith via cfe-commits
wrote:
> I forgot to say:
>
> Based on a patch by Vassil Vassilev, which was based on a patch by Bernd
> Schmidt, which was based on a patch by Reid Kleckner.
>
> On 8 August 2017 at 12:12, Richard Smith via cfe-commits
> wrote:
>>
>> Author: rsmith
>> Date: Tue Aug 8 12:12:28 2017
>> New Revision: 310401
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=310401&view=rev
>> Log:
>> PR19668, PR23034: Fix handling of move constructors and deleted copy
>> constructors when deciding whether classes should be passed indirectly.
>>
>> This fixes ABI differences between Clang and GCC:
>>
>> * Previously, Clang ignored the move constructor when making this
>>determination. It now takes the move constructor into account, per
>>https://github.com/itanium-cxx-abi/cxx-abi/pull/17 (this change may
>>seem recent, but the ABI change was agreed on the Itanium C++ ABI
>>list a long time ago).
>>
>> * Previously, Clang's behavior when the copy constructor was deleted
>>was unstable -- depending on whether the lazy declaration of the
>>copy constructor had been triggered, you might get different behavior.
>>We now eagerly declare the copy constructor whenever its deletedness
>>is unclear, and ignore deleted copy/move constructors when looking for
>>a trivial such constructor.
>>
>> This also fixes an ABI difference between Clang and MSVC:
>>
>> * If the copy constructor would be implicitly deleted (but has not been
>>lazily declared yet), for instance because the class has an rvalue
>>reference member, we would pass it directly. We now pass such a class
>>indirectly, matching MSVC.
>>
>> Modified:
>> cfe/trunk/include/clang/AST/DeclCXX.h
>> cfe/trunk/lib/AST/ASTImporter.cpp
>> cfe/trunk/lib/AST/DeclCXX.cpp
>> cfe/trunk/lib/CodeGen/CGCXXABI.cpp
>> cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
>> cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
>> cfe/trunk/lib/Sema/SemaDeclCXX.cpp
>> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>> cfe/trunk/lib/Serialization/ASTWriter.cpp
>> cfe/trunk/test/CodeGenCXX/uncopyable-args.cpp
>> cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
>>
>> Modified: cfe/trunk/include/clang/AST/DeclCXX.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=310401&r1=310400&r2=310401&view=diff
>>
>> ==
>> --- cfe/trunk/include/clang/AST/DeclCXX.h (original)
>> +++ cfe/trunk/include/clang/AST/DeclCXX.h Tue Aug 8 12:12:28 2017
>> @@ -375,6 +375,7 @@ class CXXRecordDecl : public RecordDecl
>> /// \brief These flags are \c true if a defaulted corresponding
>> special
>> /// member can't be fully analyzed without performing overload
>> resolution.
>> /// @{
>> +unsigned NeedOverloadResolutionForCopyConstructor : 1;
>> unsigned NeedOverloadResolutionForMoveConstructor : 1;
>> unsigned NeedOverloadResolutionForMoveAssignment : 1;
>> unsigned NeedOverloadResolutionForDestructor : 1;
>> @@ -383,6 +384,7 @@ class CXXRecordDecl : public RecordDecl
>> /// \brief These flags are \c true if an implicit defaulted
>> corresponding
>> /// special member would be defined as deleted.
>> /// @{
>> +unsigned DefaultedCopyConstructorIsDeleted : 1;
>> unsigned DefaultedMoveConstructorIsDeleted : 1;
>> unsigned DefaultedMoveAssignmentIsDeleted : 1;
>> unsigned DefaultedDestructorIsDeleted : 1;
>> @@ -415,6 +417,12 @@ class CXXRecordDecl : public RecordDecl
>> /// constructor.
>> unsigned HasDefaultedDefaultConstructor : 1;
>>
>> +/// \brief True if this class can be passed in a
>> non-address-preserving
>> +/// fashion (such as in registers) according to the C++ language
>> rules.
>> +/// This does not imply anything about how the ABI in use will
>> actually
>> +/// pass an object of this class.
>> +unsigned CanPassInRegisters : 1;
>> +
>> /// \brief True if a defaulted default constructor for this class
>> would
>> /// be constexpr.
>> unsigned DefaultedDefaultConstructorIsConstexpr : 1;
>> @@ -811,18 +819,50 @@ public:
>> return data().FirstFriend.isValid();
>>}
>>
>> + /// \brief \c true if a defaulted copy constructor for this class would
>> be
>> + /// deleted.
>> + bool defaultedCopyConstructorIsDeleted() const {
>> +assert((!needsOverloadResolutionForCopyConstructor() ||
>> +(data().DeclaredSpecialMembers & SMF_CopyConstructor)) &&
>> + "this property has not yet been computed by Sema");
>> +return data().DefaultedCopyConstructorIsDeleted;
>> + }
>> +
>> + /// \brief
Re: r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
I forgot to say:
Based on a patch by Vassil Vassilev, which was based on a patch by Bernd
Schmidt, which was based on a patch by Reid Kleckner.
On 8 August 2017 at 12:12, Richard Smith via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> Author: rsmith
> Date: Tue Aug 8 12:12:28 2017
> New Revision: 310401
>
> URL: http://llvm.org/viewvc/llvm-project?rev=310401&view=rev
> Log:
> PR19668, PR23034: Fix handling of move constructors and deleted copy
> constructors when deciding whether classes should be passed indirectly.
>
> This fixes ABI differences between Clang and GCC:
>
> * Previously, Clang ignored the move constructor when making this
>determination. It now takes the move constructor into account, per
>https://github.com/itanium-cxx-abi/cxx-abi/pull/17 (this change may
>seem recent, but the ABI change was agreed on the Itanium C++ ABI
>list a long time ago).
>
> * Previously, Clang's behavior when the copy constructor was deleted
>was unstable -- depending on whether the lazy declaration of the
>copy constructor had been triggered, you might get different behavior.
>We now eagerly declare the copy constructor whenever its deletedness
>is unclear, and ignore deleted copy/move constructors when looking for
>a trivial such constructor.
>
> This also fixes an ABI difference between Clang and MSVC:
>
> * If the copy constructor would be implicitly deleted (but has not been
>lazily declared yet), for instance because the class has an rvalue
>reference member, we would pass it directly. We now pass such a class
>indirectly, matching MSVC.
>
> Modified:
> cfe/trunk/include/clang/AST/DeclCXX.h
> cfe/trunk/lib/AST/ASTImporter.cpp
> cfe/trunk/lib/AST/DeclCXX.cpp
> cfe/trunk/lib/CodeGen/CGCXXABI.cpp
> cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
> cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
> cfe/trunk/lib/Sema/SemaDeclCXX.cpp
> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
> cfe/trunk/lib/Serialization/ASTWriter.cpp
> cfe/trunk/test/CodeGenCXX/uncopyable-args.cpp
> cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
>
> Modified: cfe/trunk/include/clang/AST/DeclCXX.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/
> clang/AST/DeclCXX.h?rev=310401&r1=310400&r2=310401&view=diff
>
> ==
> --- cfe/trunk/include/clang/AST/DeclCXX.h (original)
> +++ cfe/trunk/include/clang/AST/DeclCXX.h Tue Aug 8 12:12:28 2017
> @@ -375,6 +375,7 @@ class CXXRecordDecl : public RecordDecl
> /// \brief These flags are \c true if a defaulted corresponding
> special
> /// member can't be fully analyzed without performing overload
> resolution.
> /// @{
> +unsigned NeedOverloadResolutionForCopyConstructor : 1;
> unsigned NeedOverloadResolutionForMoveConstructor : 1;
> unsigned NeedOverloadResolutionForMoveAssignment : 1;
> unsigned NeedOverloadResolutionForDestructor : 1;
> @@ -383,6 +384,7 @@ class CXXRecordDecl : public RecordDecl
> /// \brief These flags are \c true if an implicit defaulted
> corresponding
> /// special member would be defined as deleted.
> /// @{
> +unsigned DefaultedCopyConstructorIsDeleted : 1;
> unsigned DefaultedMoveConstructorIsDeleted : 1;
> unsigned DefaultedMoveAssignmentIsDeleted : 1;
> unsigned DefaultedDestructorIsDeleted : 1;
> @@ -415,6 +417,12 @@ class CXXRecordDecl : public RecordDecl
> /// constructor.
> unsigned HasDefaultedDefaultConstructor : 1;
>
> +/// \brief True if this class can be passed in a
> non-address-preserving
> +/// fashion (such as in registers) according to the C++ language
> rules.
> +/// This does not imply anything about how the ABI in use will
> actually
> +/// pass an object of this class.
> +unsigned CanPassInRegisters : 1;
> +
> /// \brief True if a defaulted default constructor for this class
> would
> /// be constexpr.
> unsigned DefaultedDefaultConstructorIsConstexpr : 1;
> @@ -811,18 +819,50 @@ public:
> return data().FirstFriend.isValid();
>}
>
> + /// \brief \c true if a defaulted copy constructor for this class would
> be
> + /// deleted.
> + bool defaultedCopyConstructorIsDeleted() const {
> +assert((!needsOverloadResolutionForCopyConstructor() ||
> +(data().DeclaredSpecialMembers & SMF_CopyConstructor)) &&
> + "this property has not yet been computed by Sema");
> +return data().DefaultedCopyConstructorIsDeleted;
> + }
> +
> + /// \brief \c true if a defaulted move constructor for this class would
> be
> + /// deleted.
> + bool defaultedMoveConstructorIsDeleted() const {
> +assert((!needsOverloadResolutionForMoveConstructor() ||
> +(data().DeclaredSpecialMembers & SMF_MoveConstructor)) &&
> + "this property has not yet been computed by Sema");
> +return data().DefaultedMoveConstructorIsDeleted
r310401 - PR19668, PR23034: Fix handling of move constructors and deleted copy
Author: rsmith
Date: Tue Aug 8 12:12:28 2017
New Revision: 310401
URL: http://llvm.org/viewvc/llvm-project?rev=310401&view=rev
Log:
PR19668, PR23034: Fix handling of move constructors and deleted copy
constructors when deciding whether classes should be passed indirectly.
This fixes ABI differences between Clang and GCC:
* Previously, Clang ignored the move constructor when making this
determination. It now takes the move constructor into account, per
https://github.com/itanium-cxx-abi/cxx-abi/pull/17 (this change may
seem recent, but the ABI change was agreed on the Itanium C++ ABI
list a long time ago).
* Previously, Clang's behavior when the copy constructor was deleted
was unstable -- depending on whether the lazy declaration of the
copy constructor had been triggered, you might get different behavior.
We now eagerly declare the copy constructor whenever its deletedness
is unclear, and ignore deleted copy/move constructors when looking for
a trivial such constructor.
This also fixes an ABI difference between Clang and MSVC:
* If the copy constructor would be implicitly deleted (but has not been
lazily declared yet), for instance because the class has an rvalue
reference member, we would pass it directly. We now pass such a class
indirectly, matching MSVC.
Modified:
cfe/trunk/include/clang/AST/DeclCXX.h
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/lib/AST/DeclCXX.cpp
cfe/trunk/lib/CodeGen/CGCXXABI.cpp
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
cfe/trunk/lib/Sema/SemaDeclCXX.cpp
cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/test/CodeGenCXX/uncopyable-args.cpp
cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
Modified: cfe/trunk/include/clang/AST/DeclCXX.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=310401&r1=310400&r2=310401&view=diff
==
--- cfe/trunk/include/clang/AST/DeclCXX.h (original)
+++ cfe/trunk/include/clang/AST/DeclCXX.h Tue Aug 8 12:12:28 2017
@@ -375,6 +375,7 @@ class CXXRecordDecl : public RecordDecl
/// \brief These flags are \c true if a defaulted corresponding special
/// member can't be fully analyzed without performing overload resolution.
/// @{
+unsigned NeedOverloadResolutionForCopyConstructor : 1;
unsigned NeedOverloadResolutionForMoveConstructor : 1;
unsigned NeedOverloadResolutionForMoveAssignment : 1;
unsigned NeedOverloadResolutionForDestructor : 1;
@@ -383,6 +384,7 @@ class CXXRecordDecl : public RecordDecl
/// \brief These flags are \c true if an implicit defaulted corresponding
/// special member would be defined as deleted.
/// @{
+unsigned DefaultedCopyConstructorIsDeleted : 1;
unsigned DefaultedMoveConstructorIsDeleted : 1;
unsigned DefaultedMoveAssignmentIsDeleted : 1;
unsigned DefaultedDestructorIsDeleted : 1;
@@ -415,6 +417,12 @@ class CXXRecordDecl : public RecordDecl
/// constructor.
unsigned HasDefaultedDefaultConstructor : 1;
+/// \brief True if this class can be passed in a non-address-preserving
+/// fashion (such as in registers) according to the C++ language rules.
+/// This does not imply anything about how the ABI in use will actually
+/// pass an object of this class.
+unsigned CanPassInRegisters : 1;
+
/// \brief True if a defaulted default constructor for this class would
/// be constexpr.
unsigned DefaultedDefaultConstructorIsConstexpr : 1;
@@ -811,18 +819,50 @@ public:
return data().FirstFriend.isValid();
}
+ /// \brief \c true if a defaulted copy constructor for this class would be
+ /// deleted.
+ bool defaultedCopyConstructorIsDeleted() const {
+assert((!needsOverloadResolutionForCopyConstructor() ||
+(data().DeclaredSpecialMembers & SMF_CopyConstructor)) &&
+ "this property has not yet been computed by Sema");
+return data().DefaultedCopyConstructorIsDeleted;
+ }
+
+ /// \brief \c true if a defaulted move constructor for this class would be
+ /// deleted.
+ bool defaultedMoveConstructorIsDeleted() const {
+assert((!needsOverloadResolutionForMoveConstructor() ||
+(data().DeclaredSpecialMembers & SMF_MoveConstructor)) &&
+ "this property has not yet been computed by Sema");
+return data().DefaultedMoveConstructorIsDeleted;
+ }
+
+ /// \brief \c true if a defaulted destructor for this class would be deleted.
+ bool defaultedDestructorIsDeleted() const {
+return !data().DefaultedDestructorIsDeleted;
+ }
+
+ /// \brief \c true if we know for sure that this class has a single,
+ /// accessible, unambiguous copy constructor that is not deleted.
+ bool hasSimpleCopyConstructor() const {
+return !hasUserDeclaredCopyConstructor() &&
+ !data().D
