Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check

2019-11-27 Thread Jonathan Wakely

On 27/11/19 06:37 +0100, François Dumont wrote:

On 11/26/19 10:52 PM, Jonathan Wakely wrote:

On 26/11/19 20:07 +0100, François Dumont wrote:

Sure, I am about to do so.

However I wasn't sure about this syntax before the commit so I had 
run the new 2_neg.cc with:


make CXXFLAGS=-std=c++98 check-debug

and it worked fine and still is !


The testsuite doesn't use CXXFLAGS.


Did you mean CPPFLAGS ?


No, I mean CXXFLAGS.

I do see the option added to the compiler call in libstdc++.log file. 
And I used it to build for instance pretty-printers tests in 
_GLIBCXX_DEBUG mode with success.


Ah, it works if you run it *inside* the $target/libstdc++-v3/testsuite
directory. I usually run the tests from the parent directory.




I also try -std=gnu++98 and made sure that pch had been updated by 
re-building libstdc++ first. I just can't get the expected 
compilation error.


Maybe I need to rebuild the whole stuff to get an error...


No, you need to pass the right flags so they are used by the
testsuite. This will do it:

make -C testsuite/  check-debug 
debug_flags=unix/-D_GLIBCXX_DEBUG/-std=gnu++98/-Wsystem-headers

I'll have to keep your mail to remember it !


Just look in testsuite/Makefile to see what variables are used by the
check-debug target. That's what determines the flags used, not my
email! :-)


But since it only shows up with -Wsystem-headers, there's no point
trying to test for it.



Ok, makes sens.

Thanks





Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check

2019-11-26 Thread François Dumont

On 11/26/19 10:52 PM, Jonathan Wakely wrote:

On 26/11/19 20:07 +0100, François Dumont wrote:

Sure, I am about to do so.

However I wasn't sure about this syntax before the commit so I had 
run the new 2_neg.cc with:


make CXXFLAGS=-std=c++98 check-debug

and it worked fine and still is !


The testsuite doesn't use CXXFLAGS.


Did you mean CPPFLAGS ?

I do see the option added to the compiler call in libstdc++.log file. 
And I used it to build for instance pretty-printers tests in 
_GLIBCXX_DEBUG mode with success.




I also try -std=gnu++98 and made sure that pch had been updated by 
re-building libstdc++ first. I just can't get the expected 
compilation error.


Maybe I need to rebuild the whole stuff to get an error...


No, you need to pass the right flags so they are used by the
testsuite. This will do it:

make -C testsuite/  check-debug 
debug_flags=unix/-D_GLIBCXX_DEBUG/-std=gnu++98/-Wsystem-headers

I'll have to keep your mail to remember it !


But since it only shows up with -Wsystem-headers, there's no point
trying to test for it.



Ok, makes sens.

Thanks



Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check

2019-11-26 Thread Jonathan Wakely

On 26/11/19 20:07 +0100, François Dumont wrote:

On 11/26/19 1:21 PM, Jonathan Wakely wrote:

On 26/11/19 12:33 +0100, Stephan Bergmann wrote:

On 22/11/2019 18:59, Jonathan Wakely wrote:

On 22/11/19 18:38 +0100, François Dumont wrote:
    I noticed that we are not checking that iterators are not 
singular in valid_range. Moreover __check_singular signature 
for pointers is not intercepting all kind of pointers in terms 
of qualification.


    I'd like to commit it next week but considering we are in 
stage 3 I need proper acceptance.


    * include/debug/functions.h: Remove  include.
    (__check_singular_aux, __check_singular): Move...
    * include/debug/helper_functions.h:
    (__check_singular_aux, __check_singular): ...here.
    (__valid_range_aux): Adapt to use latter.
    * testsuite/25_algorithms/copy/debug/2_neg.cc: New.

Tested under Linux x86_64 normal and debug modes.


OK for trunk, thanks.


The curly braces...

diff --git a/libstdc++-v3/include/debug/helper_functions.h 
b/libstdc++-v3/include/debug/helper_functions.h

index c3e7478f649..5a858754875 100644
--- a/libstdc++-v3/include/debug/helper_functions.h
+++ b/libstdc++-v3/include/debug/helper_functions.h

[...]

@@ -138,14 +156,23 @@ namespace __gnu_debug
    inline bool
    __valid_range_aux(_InputIterator __first, _InputIterator __last,
  std::input_iterator_tag)
-    { return true; }
+    {
+  if (__first != __last)
+    return !__check_singular(__first) && !__check_singular(__last);
+
+  return true;
+    }
  template
    _GLIBCXX_CONSTEXPR
    inline bool
    __valid_range_aux(_InputIterator __first, _InputIterator __last,
  std::random_access_iterator_tag)
-    { return __first <= __last; }
+    {
+  return
+    __valid_range_aux(__first, __last, std::input_iterator_tag{})


...^^^ here...


+    && __first <= __last;
+    }
  /** We have iterators, so figure out what kind of iterators they are
   *  to see if we can check the range ahead of time.
@@ -167,6 +194,9 @@ namespace __gnu_debug
  typename _Distance_traits<_InputIterator>::__type& 
__dist,

  std::__false_type)
    {
+  if (!__valid_range_aux(__first, __last, 
std::input_iterator_tag{}))


...and ^^^ here are not allowed pre C++11.  Replacing those with

 std::input_iterator_tag()

should fix it.


Indeed. We should also have tests that use "-std=gnu++98
-D_GLIBCXX_DEBUG" so they'd catch this.

François, can you take care of the fix please?




Sure, I am about to do so.

However I wasn't sure about this syntax before the commit so I had run 
the new 2_neg.cc with:


make CXXFLAGS=-std=c++98 check-debug

and it worked fine and still is !


The testsuite doesn't use CXXFLAGS.


I also try -std=gnu++98 and made sure that pch had been updated by 
re-building libstdc++ first. I just can't get the expected compilation 
error.


Maybe I need to rebuild the whole stuff to get an error...


No, you need to pass the right flags so they are used by the
testsuite. This will do it:

make -C testsuite/  check-debug 
debug_flags=unix/-D_GLIBCXX_DEBUG/-std=gnu++98/-Wsystem-headers

But since it only shows up with -Wsystem-headers, there's no point
trying to test for it.



Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check

2019-11-26 Thread François Dumont

On 11/26/19 9:49 PM, Stephan Bergmann wrote:

On 26/11/2019 20:07, François Dumont wrote:
However I wasn't sure about this syntax before the commit so I had 
run the new 2_neg.cc with:


make CXXFLAGS=-std=c++98 check-debug

and it worked fine and still is !

I also try -std=gnu++98 and made sure that pch had been updated by 
re-building libstdc++ first. I just can't get the expected 
compilation error.


Maybe I need to rebuild the whole stuff to get an error...


I saw the error with recent Clang and -std=gnu++98.  Recent GCC indeed 
seems to give at most a warning.




Ok, thanks for the feedback, gcc might be more permissive then.

Whatever I committed the fix an hour ago.



Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check

2019-11-26 Thread Stephan Bergmann

On 26/11/2019 20:07, François Dumont wrote:
However I wasn't sure about this syntax before the commit so I had run 
the new 2_neg.cc with:


make CXXFLAGS=-std=c++98 check-debug

and it worked fine and still is !

I also try -std=gnu++98 and made sure that pch had been updated by 
re-building libstdc++ first. I just can't get the expected compilation 
error.


Maybe I need to rebuild the whole stuff to get an error...


I saw the error with recent Clang and -std=gnu++98.  Recent GCC indeed 
seems to give at most a warning.




Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check

2019-11-26 Thread François Dumont

On 11/26/19 1:21 PM, Jonathan Wakely wrote:

On 26/11/19 12:33 +0100, Stephan Bergmann wrote:

On 22/11/2019 18:59, Jonathan Wakely wrote:

On 22/11/19 18:38 +0100, François Dumont wrote:
    I noticed that we are not checking that iterators are not 
singular in valid_range. Moreover __check_singular signature for 
pointers is not intercepting all kind of pointers in terms of 
qualification.


    I'd like to commit it next week but considering we are in stage 
3 I need proper acceptance.


    * include/debug/functions.h: Remove  include.
    (__check_singular_aux, __check_singular): Move...
    * include/debug/helper_functions.h:
    (__check_singular_aux, __check_singular): ...here.
    (__valid_range_aux): Adapt to use latter.
    * testsuite/25_algorithms/copy/debug/2_neg.cc: New.

Tested under Linux x86_64 normal and debug modes.


OK for trunk, thanks.


The curly braces...

diff --git a/libstdc++-v3/include/debug/helper_functions.h 
b/libstdc++-v3/include/debug/helper_functions.h

index c3e7478f649..5a858754875 100644
--- a/libstdc++-v3/include/debug/helper_functions.h
+++ b/libstdc++-v3/include/debug/helper_functions.h

[...]

@@ -138,14 +156,23 @@ namespace __gnu_debug
    inline bool
    __valid_range_aux(_InputIterator __first, _InputIterator __last,
  std::input_iterator_tag)
-    { return true; }
+    {
+  if (__first != __last)
+    return !__check_singular(__first) && !__check_singular(__last);
+
+  return true;
+    }
  template
    _GLIBCXX_CONSTEXPR
    inline bool
    __valid_range_aux(_InputIterator __first, _InputIterator __last,
  std::random_access_iterator_tag)
-    { return __first <= __last; }
+    {
+  return
+    __valid_range_aux(__first, __last, std::input_iterator_tag{})


...^^^ here...


+    && __first <= __last;
+    }
  /** We have iterators, so figure out what kind of iterators they are
   *  to see if we can check the range ahead of time.
@@ -167,6 +194,9 @@ namespace __gnu_debug
  typename _Distance_traits<_InputIterator>::__type& 
__dist,

  std::__false_type)
    {
+  if (!__valid_range_aux(__first, __last, 
std::input_iterator_tag{}))


...and ^^^ here are not allowed pre C++11.  Replacing those with

 std::input_iterator_tag()

should fix it.


Indeed. We should also have tests that use "-std=gnu++98
-D_GLIBCXX_DEBUG" so they'd catch this.

François, can you take care of the fix please?




Sure, I am about to do so.

However I wasn't sure about this syntax before the commit so I had run 
the new 2_neg.cc with:


make CXXFLAGS=-std=c++98 check-debug

and it worked fine and still is !

I also try -std=gnu++98 and made sure that pch had been updated by 
re-building libstdc++ first. I just can't get the expected compilation 
error.


Maybe I need to rebuild the whole stuff to get an error...

Sorry



Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check

2019-11-26 Thread Jonathan Wakely

On 26/11/19 12:33 +0100, Stephan Bergmann wrote:

On 22/11/2019 18:59, Jonathan Wakely wrote:

On 22/11/19 18:38 +0100, François Dumont wrote:
    I noticed that we are not checking that iterators are not 
singular in valid_range. Moreover __check_singular signature for 
pointers is not intercepting all kind of pointers in terms of 
qualification.


    I'd like to commit it next week but considering we are in 
stage 3 I need proper acceptance.


    * include/debug/functions.h: Remove  include.
    (__check_singular_aux, __check_singular): Move...
    * include/debug/helper_functions.h:
    (__check_singular_aux, __check_singular): ...here.
    (__valid_range_aux): Adapt to use latter.
    * testsuite/25_algorithms/copy/debug/2_neg.cc: New.

Tested under Linux x86_64 normal and debug modes.


OK for trunk, thanks.


The curly braces...


diff --git a/libstdc++-v3/include/debug/helper_functions.h 
b/libstdc++-v3/include/debug/helper_functions.h
index c3e7478f649..5a858754875 100644
--- a/libstdc++-v3/include/debug/helper_functions.h
+++ b/libstdc++-v3/include/debug/helper_functions.h

[...]

@@ -138,14 +156,23 @@ namespace __gnu_debug
inline bool
__valid_range_aux(_InputIterator __first, _InputIterator __last,
  std::input_iterator_tag)
-{ return true; }
+{
+  if (__first != __last)
+   return !__check_singular(__first) && !__check_singular(__last);
+
+  return true;
+}
  template
_GLIBCXX_CONSTEXPR
inline bool
__valid_range_aux(_InputIterator __first, _InputIterator __last,
  std::random_access_iterator_tag)
-{ return __first <= __last; }
+{
+  return
+   __valid_range_aux(__first, __last, std::input_iterator_tag{})


...^^^ here...


+   && __first <= __last;
+}
  /** We have iterators, so figure out what kind of iterators they are
   *  to see if we can check the range ahead of time.
@@ -167,6 +194,9 @@ namespace __gnu_debug
  typename _Distance_traits<_InputIterator>::__type& __dist,
  std::__false_type)
{
+  if (!__valid_range_aux(__first, __last, std::input_iterator_tag{}))


...and ^^^ here are not allowed pre C++11.  Replacing those with

 std::input_iterator_tag()

should fix it.


Indeed. We should also have tests that use "-std=gnu++98
-D_GLIBCXX_DEBUG" so they'd catch this.

François, can you take care of the fix please?




Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check

2019-11-26 Thread Stephan Bergmann

On 22/11/2019 18:59, Jonathan Wakely wrote:

On 22/11/19 18:38 +0100, François Dumont wrote:
    I noticed that we are not checking that iterators are not singular 
in valid_range. Moreover __check_singular signature for pointers is 
not intercepting all kind of pointers in terms of qualification.


    I'd like to commit it next week but considering we are in stage 3 
I need proper acceptance.


    * include/debug/functions.h: Remove  include.
    (__check_singular_aux, __check_singular): Move...
    * include/debug/helper_functions.h:
    (__check_singular_aux, __check_singular): ...here.
    (__valid_range_aux): Adapt to use latter.
    * testsuite/25_algorithms/copy/debug/2_neg.cc: New.

Tested under Linux x86_64 normal and debug modes.


OK for trunk, thanks.


The curly braces...


diff --git a/libstdc++-v3/include/debug/helper_functions.h 
b/libstdc++-v3/include/debug/helper_functions.h
index c3e7478f649..5a858754875 100644
--- a/libstdc++-v3/include/debug/helper_functions.h
+++ b/libstdc++-v3/include/debug/helper_functions.h

[...]

@@ -138,14 +156,23 @@ namespace __gnu_debug
 inline bool
 __valid_range_aux(_InputIterator __first, _InputIterator __last,
  std::input_iterator_tag)
-{ return true; }
+{
+  if (__first != __last)
+   return !__check_singular(__first) && !__check_singular(__last);
+
+  return true;
+}
 
   template

 _GLIBCXX_CONSTEXPR
 inline bool
 __valid_range_aux(_InputIterator __first, _InputIterator __last,
  std::random_access_iterator_tag)
-{ return __first <= __last; }
+{
+  return
+   __valid_range_aux(__first, __last, std::input_iterator_tag{})


...^^^ here...


+   && __first <= __last;
+}
 
   /** We have iterators, so figure out what kind of iterators they are

*  to see if we can check the range ahead of time.
@@ -167,6 +194,9 @@ namespace __gnu_debug
  typename _Distance_traits<_InputIterator>::__type& __dist,
  std::__false_type)
 {
+  if (!__valid_range_aux(__first, __last, std::input_iterator_tag{}))


...and ^^^ here are not allowed pre C++11.  Replacing those with

  std::input_iterator_tag()

should fix it.


+   return false;
+
   __dist = __get_distance(__first, __last);
   switch (__dist.second)
{




Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check

2019-11-22 Thread Jonathan Wakely

On 22/11/19 18:38 +0100, François Dumont wrote:

Hi

    I noticed that we are not checking that iterators are not singular 
in valid_range. Moreover __check_singular signature for pointers is 
not intercepting all kind of pointers in terms of qualification.


    I'd like to commit it next week but considering we are in stage 3 
I need proper acceptance.


    * include/debug/functions.h: Remove  include.
    (__check_singular_aux, __check_singular): Move...
    * include/debug/helper_functions.h:
    (__check_singular_aux, __check_singular): ...here.
    (__valid_range_aux): Adapt to use latter.
    * testsuite/25_algorithms/copy/debug/2_neg.cc: New.

Tested under Linux x86_64 normal and debug modes.


OK for trunk, thanks.