Re: [PATCH] testsuite: Robustify aarch64/simd tests against more aggressive DCE
Hi,
> Sorry, just realised I'd never replied to this.
No worries! I also took a very long time to reply, sorry.
> The problem is that we only enforce lane bounds via calls to
> __builtin_aarch64_im_lane_boundsi. In previous releases, the check
> only happend at RTL expansion time, so the check would be skipped if
> any gimple pass removed the call. Now we do the checking during
> folding, but that still misses cases. E.g., compare the -O0 and -O1
> behaviour for:
>
> #include
>
> void f(int32x4_t *p0, int16x8_t *p1) {
> vqdmlal_high_laneq_s16(p0[0], p1[0], p1[1], -1);
> //p0[0] = vqdmlal_high_laneq_s16(p0[0], p1[0], p1[1], -1);
> }
>
> -O0 gives the error but -O1 doesn't [https://godbolt.org/z/1KosTY43T].
> The -O1 behaviour here is wrong: badly-formed calls should be rejected
> with a diagnostic even if the calls are unused. Clang gets this right
> in both cases [https://godbolt.org/z/EGxs8jq97].
>
> I think keeping the lhs-free calls is important for making sure that
> the -O0 behaviour doesn't regress without the DCE.
>
> Your DCE will regress it, but that's the fault of the arm_neon.h
> implementation rather than the fault of your pass. Having the
> tests but XFAILing them seems like the best way of dealing with that.
> Hopefully we'll then see some progression if the arm_neon.h implementation
> is improved in future.
Ok, thanks for the clarification, I now understand why you prefer to
keep the existing tests.
The initial goal of the patch is to minimize our changes in the tests
results by having the tests PASS both with our internal compiler and
with master. I can apply the modifications you are suggesting (i.e.
adding new calls with assignments) if you think this is an improvement
over the current state (but internally, we would still need to mark
existing tests as XFAIL).
Thanks!
Marc
Re: [PATCH] testsuite: add missing dg-require-effective-target fpic
Marc Poulhiès writes:
> Require effective target fpic for newly added test.
>
> gcc/testsuite/
> * g++.dg/ext/visibility/visibility-local-extern1.C: Add missing
> dg-require-effective-target fpic.
>
> Tested on x86_64-linux. Ok for master?
>
> ---
> gcc/testsuite/g++.dg/ext/visibility/visibility-local-extern1.C | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/gcc/testsuite/g++.dg/ext/visibility/visibility-local-extern1.C
> b/gcc/testsuite/g++.dg/ext/visibility/visibility-local-extern1.C
> index 40c20199d0c..6fb1cc7f381 100644
> --- a/gcc/testsuite/g++.dg/ext/visibility/visibility-local-extern1.C
> +++ b/gcc/testsuite/g++.dg/ext/visibility/visibility-local-extern1.C
> @@ -1,6 +1,7 @@
> // PR c++/103291
> // { dg-additional-options -fpic }
> // { dg-final { scan-assembler-not "@GOTPCREL" } }
> +// { dg-require-effective-target fpic }
>
> #pragma GCC visibility push(hidden)
ping ?
Marc
libstdc++: Fix testsuite for skipping gdb tests on remote/non-native target
This fixes an incorrect invocation of gdb on remote targets where DejaGNU would
try to run host's gdb in remote target simulator.
gdb-test skips the testing when target is remote or non native but the gdb
version check function does not.
libstdc++-v3/ChangeLog:
* testsuite/lib/gdb-test.exp (gdb_batch_check): Exit if non native or
remote target.
commit 0c4ae4ff46b1d7633f1e06f57d348b5817b8f640
Author: Jonathan Wakely
Date: Tue Jul 20 12:35:37 2021
libstdc++: Add more tests for filesystem::create_directory [PR101510]
Signed-off-by: Jonathan Wakely
libstdc++-v3/ChangeLog:
PR libstdc++/101510
* src/c++17/fs_ops.cc (create_dir): Adjust whitespace.
* testsuite/27_io/filesystem/operations/create_directory.cc:
Test creating directory with name of existing symlink to
directory.
* testsuite/experimental/filesystem/operations/create_directory.cc:
Likewise.
diff --git a/libstdc++-v3/src/c++17/fs_ops.cc b/libstdc++-v3/src/c++17/fs_ops.cc
index 66207ae5e44..cec76446f06 100644
--- a/libstdc++-v3/src/c++17/fs_ops.cc
+++ b/libstdc++-v3/src/c++17/fs_ops.cc
@@ -577,8 +577,7 @@ namespace
{
bool created = false;
#ifdef _GLIBCXX_HAVE_SYS_STAT_H
-posix::mode_t mode
- = static_cast>(perm);
+posix::mode_t mode = static_cast>(perm);
if (posix::mkdir(p.c_str(), mode))
{
const int err = errno;
diff --git
a/libstdc++-v3/testsuite/27_io/filesystem/operations/create_directory.cc
b/libstdc++-v3/testsuite/27_io/filesystem/operations/create_directory.cc
index a0e50471275..256621481d7 100644
--- a/libstdc++-v3/testsuite/27_io/filesystem/operations/create_directory.cc
+++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/create_directory.cc
@@ -54,6 +54,33 @@ test01()
b = create_directory(p);
VERIFY( !b );
+ auto f = p/"file";
+ std::ofstream{f} << "create file";
+ b = create_directory(f, ec);
+ VERIFY( ec == std::errc::file_exists );
+ VERIFY( !b );
+ try
+ {
+create_directory(f);
+VERIFY( false );
+ }
+ catch (const fs::filesystem_error& e)
+ {
+VERIFY( e.code() == std::errc::file_exists );
+VERIFY( e.path1() == f );
+ }
+
+ // PR libstdc++/101510 create_directory on an existing symlink to a directory
+ fs::create_directory(p/"dir");
+ auto link = p/"link";
+ fs::create_directory_symlink("dir", link);
+ ec = bad_ec;
+ b = fs::create_directory(link, ec);
+ VERIFY( !b );
+ VERIFY( !ec );
+ b = fs::create_directory(link);
+ VERIFY( !b );
+
remove_all(p, ec);
}
diff --git
a/libstdc++-v3/testsuite/experimental/filesystem/operations/create_directory.cc
b/libstdc++-v3/testsuite/experimental/filesystem/operations/create_directory.cc
index ee2a74b8803..39f95b61a45 100644
---
a/libstdc++-v3/testsuite/experimental/filesystem/operations/create_directory.cc
+++
b/libstdc++-v3/testsuite/experimental/filesystem/operations/create_directory.cc
@@ -46,12 +46,40 @@ test01()
VERIFY( exists(p) );
// Test existing path (libstdc++/71036).
+ ec = make_error_code(std::errc::invalid_argument);
b = create_directory(p, ec);
VERIFY( !ec );
VERIFY( !b );
b = create_directory(p);
VERIFY( !b );
+ auto f = p/"file";
+ std::ofstream{f} << "create file";
+ b = create_directory(f, ec);
+ VERIFY( ec == std::errc::file_exists );
+ VERIFY( !b );
+ try
+ {
+create_directory(f);
+VERIFY( false );
+ }
+ catch (const fs::filesystem_error& e)
+ {
+VERIFY( e.code() == std::errc::file_exists );
+VERIFY( e.path1() == f );
+ }
+
+ // PR libstdc++/101510 create_directory on an existing symlink to a directory
+ fs::create_directory(p/"dir");
+ auto link = p/"link";
+ fs::create_directory_symlink("dir", link);
+ ec = make_error_code(std::errc::invalid_argument);
+ b = fs::create_directory(link, ec);
+ VERIFY( !b );
+ VERIFY( !ec );
+ b = fs::create_directory(link);
+ VERIFY( !b );
+
remove_all(p, ec);
}
Re: [NEWS] libstdc++: Fix testsuite for skipping gdb tests on remote/non-native target
With the correct patch attached, sorry for the incorrect previous one !
Marc
- Original Message -
> From: "gcc-patches"
> To: "gcc-patches" , "libstdc++"
>
> Cc: "Luc Michel"
> Sent: Tuesday, July 20, 2021 4:12:16 PM
> Subject: [NEWS] libstdc++: Fix testsuite for skipping gdb tests on
> remote/non-native target
> This fixes an incorrect invocation of gdb on remote targets where DejaGNU
> would
> try to run host's gdb in remote target simulator.
> gdb-test skips the testing when target is remote or non native but the gdb
> version check function does not.
>
> libstdc++-v3/ChangeLog:
>* testsuite/lib/gdb-test.exp (gdb_batch_check): Exit if non native or
> remote
> target.
diff --git a/libstdc++-v3/testsuite/lib/gdb-test.exp b/libstdc++-v3/testsuite/lib/gdb-test.exp
index af20c85e5a0..0ec9ac46c68 100644
--- a/libstdc++-v3/testsuite/lib/gdb-test.exp
+++ b/libstdc++-v3/testsuite/lib/gdb-test.exp
@@ -244,6 +244,8 @@ proc gdb-test { marker {selector {}} {load_xmethods 0} } {
# Invoke gdb with a command and pattern-match the output.
proc gdb_batch_check {command pattern} {
+if { ![isnative] || [is_remote target] } { return 0 }
+
set gdb_name $::env(GUALITY_GDB_NAME)
set cmd "$gdb_name -nw -nx -quiet -batch -ex \"$command\""
send_log "Spawning: $cmd\n"
Re: [NEWS] libstdc++: Fix testsuite for skipping gdb tests on remote/non-native target
- Original Message - > From: "Jonathan Wakely" > To: "Marc Poulhies" > Cc: "libstdc++" , "gcc-patches" > , "Luc Michel" > Sent: Wednesday, July 21, 2021 5:53:52 PM > Subject: Re: [NEWS] libstdc++: Fix testsuite for skipping gdb tests on > remote/non-native target > Thanks for the patch. I agree we should skip the version checks, not > only the actual tests. But I wonder whether we want to do that in > xmethods.exp and prettyprinters.exp rather than in the gdb_batch_check > proc. Or maybe like this instead: > > I don't think it really makes much difference, I'm just unsure what is > "cleaner" and more consistent with DG conventions and/or the rest of > the gdb-test.exp file. Here are a bit more information on the issue we are having. While trying to understand why the testsuite is taking a long time to execute, we (Luc in Cc) discovered that the logs contain: Spawning: gdb -nw -nx -quiet -batch -ex "python print(gdb.lookup_global_symbol)" spawn -ignore SIGHUP kvx-mppa --unnamed-log --bootcluster=node0 --no-trap --no-gdb-attach --dcache-no-check -- gdb -nw -nx -quiet -batch -ex python print(gdb.lookup_global_symbol) UNSUPPORTED: prettyprinters.exp testcase /work1/mpoulhies/tools-csw/gcc/libstdc++-v3/testsuite/libstdc++-prettyprinters/prettyprinters.exp completed in 613 seconds kvx-mppa being a simulator and gdb the host (x86) binary. The strangest part being that running the command by hand will fail immediately, but when DG is running the tests, it waits until the timeout is reached: we don't understand this behavior, but we get several processes probably waiting to be joined.. I don't have a strong opinion here as I'm really no DG expert. I find the filtering in gdb-test maybe more robust as it prevents any error like the above. Having the test in prettyprinters/xmethod allows for quicker exit (saves 15s here), but I don't see that as a strong argument. Thanks for the review, Marc
Re: [PATCH] testsuite: Robustify aarch64/simd tests against more aggressive DCE
Eric Botcazou writes:
>> The new variables seem to be unused, so I think slightly stronger
>> DCE could remove the calls even after the patch. Perhaps the containing
>> functions should take an int32x4_t *ptr or something, with the calls
>> assigning to different ptr[] indices.
>
> We run a minimal DCE pass at -O0 in our compiler to eliminate all the garbage
> generated by the gimplifier for variable-sized types (people care about code
> size at -O0 in specific contexts) but it does not touch anything written by
> the user (and debugging is unaffected of course). Given that the builtins
> are
> pure functions and the arguments have no side effects, it eliminates the
> calls, but adding a LHS blocks that because this minimal DCE pass preserves
> anything user-related, in particular assignments to user variables.
>
>> I think it would be better to do that using new calls though,
>> and xfail the existing ones when they no longer work. For example:
>>
>> /* { dg-error "lane -1 out of range 0 - 7" "" {target *-*-*} 0 } */
>> vqdmlal_high_laneq_s16 (int32x4_a, int16x8_b, int16x8_c, -1);
>> /* { dg-error "lane -1 out of range 0 - 7" "" {target *-*-*} 0 } */
>> ptr[0] = vqdmlal_high_laneq_s16 (int32x4_a, int16x8_b, int16x8_c, -1);
>>
>> That way we don't lose the existing tests.
>
> Frankly I'm not quite sure of what we can lose by adding a LHS here, can you
> elaborate a bit? We would need a solution that works out of the box with our
> compiler in the future, i.e. without having to tweak 50 testcases again.
Hi Richard,
Thank for your reply !
As Éric, I'm also wondering why having LHS in the existing tests would
make us loose them. I guess I'm not familiar enough with this part of
the testsuite and I'm missing something.
Thanks,
Marc
