Switch to std::atomic?

classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Switch to std::atomic?

Rik-4
jwe,

I have been looking at the performance of Octave and one of the slow points
has been the atomic operations.  This led me to look at the actual code in
oct-refcount.h quoted below.

#if (defined (OCTAVE_ENABLE_ATOMIC_REFCOUNT) \
     && (defined (__GNUC__) || defined (_MSC_VER)))

#  if defined (__GNUC__)

#    define OCTAVE_ATOMIC_INCREMENT(x) __sync_add_and_fetch (x,  1)
#    define OCTAVE_ATOMIC_DECREMENT(x) __sync_add_and_fetch (x, -1)
#    define OCTAVE_ATOMIC_POST_INCREMENT(x) __sync_fetch_and_add (x,  1)
#    define OCTAVE_ATOMIC_POST_DECREMENT(x) __sync_fetch_and_add (x, -1)

#  elif defined (_MSC_VER)

#    include <intrin.h>

#    define OCTAVE_ATOMIC_INCREMENT(x)                  \
  _InterlockedIncrement (static_cast<long *> (x))

#    define OCTAVE_ATOMIC_DECREMENT(x)                  \
  _InterlockedDecrement (static_cast<long *> (x))

#    define OCTAVE_ATOMIC_POST_INCREMENT(x)             \
  _InterlockedExchangeAdd (static_cast<long *> (x))

#    define OCTAVE_ATOMIC_POST_DECREMENT(x)             \
  _InterlockedExchangeAdd (static_cast<long *> (x))

#  endif

So, only with gcc (or something close to it like clang) or on Windows do we
have true atomic instructions.  If these conditions aren't met then
ordinary, non-atomic operations are substituted which won't work if
plotting or the GUI are used.

Would it make sense to switch to using std::atomic<octave_idx_type>?  In
that case, Octave would be portable to any machine with a valid C++ runtime
library, and we certainly require that.  The std::atomic class overloads
the (pre-, post-)(increment, decrement) operators so one could write more
readable code with count++ rather than OCTAVE_ATOMIC_POST_INCREMENT(count).

Just an idea, but I don't want to go ahead if there is some larger reason
we shouldn't do this.

--Rik

Reply | Threaded
Open this post in threaded view
|

Re: Switch to std::atomic?

John W. Eaton
Administrator
On 9/13/19 5:29 PM, Rik wrote:

> Would it make sense to switch to using std::atomic<octave_idx_type>?  In
> that case, Octave would be portable to any machine with a valid C++ runtime
> library, and we certainly require that.  The std::atomic class overloads
> the (pre-, post-)(increment, decrement) operators so one could write more
> readable code with count++ rather than OCTAVE_ATOMIC_POST_INCREMENT(count).

Yes, now that it is a standard C++ feature, we should be using that.
For a simple transition, can we first define our refcount class using
std::atomic<T>?

Would you like to make this change?  If not, I can look at it.

jwe


Reply | Threaded
Open this post in threaded view
|

Re: Switch to std::atomic?

John W. Eaton
Administrator
On 9/25/19 10:34 AM, John W. Eaton wrote:

> On 9/13/19 5:29 PM, Rik wrote:
>
>> Would it make sense to switch to using std::atomic<octave_idx_type>?  In
>> that case, Octave would be portable to any machine with a valid C++
>> runtime
>> library, and we certainly require that.  The std::atomic class overloads
>> the (pre-, post-)(increment, decrement) operators so one could write more
>> readable code with count++ rather than
>> OCTAVE_ATOMIC_POST_INCREMENT(count).
>
> Yes, now that it is a standard C++ feature, we should be using that. For
> a simple transition, can we first define our refcount class using
> std::atomic<T>?
>
> Would you like to make this change?  If not, I can look at it.
I was curious to see what it would take and came up with the following
change.  I think this is OK as far as it goes, but I haven't pushed it.

Notes and questions:

   * The C11 atomic_fetch_add and atomic_fetch_sub functions were
required to handle the tricky way that the dim_vector class stores the
reference count.  I believe that to do this job in C++ requires the
atomic_ref object that is a new feature in C++20.  Either that or use a
separate reference count variable instead of storing it in the array of
values, but then as the comment in dim-vector.h says, we will need two
allocations, one for the rep object and one for the array that it will
then contain.

   * Some reference counts use int and some use octave_idx_type.  Maybe
we should always use the same type?  Should it be int or octave_idx_type
or size_t?

   * The copy constructor for the cdef_class_rep object looks wrong to
me.  Instead of copying the reference count, shouldn't the count for the
new object be 1?  But I'm not sure I understand the way this code is
supposed to work so I left it alone and provided a copy constructor for
the refcount class.

jwe


refcount-diffs.txt (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Is fork() broken in octave 5.1 ?

Dr. K. nick
...or am I getting something wrong here?

Dear folks,

this simple script (forktest.m)

A=23;
[pid, msg]=fork();
if (pid > 0)
  pid
  msg
endif

gives me the error

>> forktest

error: fork: cannot be called from command line
error: called from
    forktest at line 2 column 11

>>

Tested it on Arch Linux as well as Manjaro (64 bit) with all recent
updates installed. Any idea?

Marcin



Reply | Threaded
Open this post in threaded view
|

Re: Is fork() broken in octave 5.1 ?

Mike Miller-4
On Thu, Sep 26, 2019 at 17:37:13 +0200, Kay Nick wrote:

> ...or am I getting something wrong here?
>
> Dear folks,
>
> this simple script (forktest.m)
>
> A=23;
> [pid, msg]=fork();
> if (pid > 0)
>   pid
>   msg
> endif
>
> gives me the error
>
> >> forktest
>
> error: fork: cannot be called from command line
> error: called from
>     forktest at line 2 column 11
>
> >>
>
> Tested it on Arch Linux as well as Manjaro (64 bit) with all recent
> updates installed. Any idea?
The error message says "command line", but includes scripts as well.
This error was added intentionally, so it looks to me like the function
is working correctly. Have you tried turning forktest.m into a function
file?

--
mike

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Is fork() broken in octave 5.1 ?

Dr. K. nick
ftest.m :
function ret=ftest()
  [pid, msg]=fork();
  if (pid > 0)
    pid
    msg
  endif
  ret=0;


forktest.m
A=23;
ftest()

gives me

>> forktest
pid =  3345
msg =
ans = ans0
 = 0
>>

which is pretty much what I'd expect. Maybe we should mention this in
the documentation or in the error message... (only allowed in a function)

> The error message says "command line", but includes scripts as well.
> This error was added intentionally, so it looks to me like the function
> is working correctly.
Nevertheless I find this behavior not intuitive... What is the reason
for preventing fork() from being used in a simple script?

Marcin


On 26.09.19 19:04, Mike Miller wrote:

> On Thu, Sep 26, 2019 at 17:37:13 +0200, Kay Nick wrote:
>> ...or am I getting something wrong here?
>>
>> Dear folks,
>>
>> this simple script (forktest.m)
>>
>> A=23;
>> [pid, msg]=fork();
>> if (pid > 0)
>>   pid
>>   msg
>> endif
>>
>> gives me the error
>>
>>>> forktest
>> error: fork: cannot be called from command line
>> error: called from
>>     forktest at line 2 column 11
>>
>> Tested it on Arch Linux as well as Manjaro (64 bit) with all recent
>> updates installed. Any idea?
> The error message says "command line", but includes scripts as well.
> This error was added intentionally, so it looks to me like the function
> is working correctly. Have you tried turning forktest.m into a function
> file?
>

Reply | Threaded
Open this post in threaded view
|

Re: Is fork() broken in octave 5.1 ?

Mike Miller-4
On Thu, Sep 26, 2019 at 19:27:55 +0200, Kay Nick wrote:
> Nevertheless I find this behavior not intuitive... What is the reason
> for preventing fork() from being used in a simple script?

It is related to this (still open) bug report:
https://savannah.gnu.org/bugs/?45625

--
mike

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Is fork() broken in octave 5.1 ?

John W. Eaton
Administrator
On 9/26/19 1:42 PM, Mike Miller wrote:
> On Thu, Sep 26, 2019 at 19:27:55 +0200, Kay Nick wrote:
>> Nevertheless I find this behavior not intuitive... What is the reason
>> for preventing fork() from being used in a simple script?
>
> It is related to this (still open) bug report:
> https://savannah.gnu.org/bugs/?45625

Yes, the check was added to prevent simple calls to fork at the command
line from creating two copies of Octave both competing for input from
the terminal and possibly confusing users.  Once this situation occurs,
it can be difficult to recover from without killing the session.

The check that was added doesn't distinguish between the command line
and a script evaluated from the command line.  A script executes in the
scope of the caller and the check was for execution at the top-level
scope rather than checking the depth of the call stack.

Maybe we could change the condition so that it will allow fork if it is
executed from a script but not directly from the command line.

jwe



Reply | Threaded
Open this post in threaded view
|

Re: Switch to std::atomic?

John W. Eaton
Administrator
In reply to this post by John W. Eaton
On 9/25/19 6:10 PM, John W. Eaton wrote:

> On 9/25/19 10:34 AM, John W. Eaton wrote:
>> On 9/13/19 5:29 PM, Rik wrote:
>>
>>> Would it make sense to switch to using std::atomic<octave_idx_type>?  In
>>> that case, Octave would be portable to any machine with a valid C++
>>> runtime
>>> library, and we certainly require that.  The std::atomic class overloads
>>> the (pre-, post-)(increment, decrement) operators so one could write
>>> more
>>> readable code with count++ rather than
>>> OCTAVE_ATOMIC_POST_INCREMENT(count).
>>
>> Yes, now that it is a standard C++ feature, we should be using that.
>> For a simple transition, can we first define our refcount class using
>> std::atomic<T>?
>>
>> Would you like to make this change?  If not, I can look at it.
>
> I was curious to see what it would take and came up with the following
> change.  I think this is OK as far as it goes, but I haven't pushed it.
>
> Notes and questions:
>
>    * The C11 atomic_fetch_add and atomic_fetch_sub functions were
> required to handle the tricky way that the dim_vector class stores the
> reference count.  I believe that to do this job in C++ requires the
> atomic_ref object that is a new feature in C++20.  Either that or use a
> separate reference count variable instead of storing it in the array of
> values, but then as the comment in dim-vector.h says, we will need two
> allocations, one for the rep object and one for the array that it will
> then contain.
>
>    * Some reference counts use int and some use octave_idx_type.  Maybe
> we should always use the same type?  Should it be int or octave_idx_type
> or size_t?
>
>    * The copy constructor for the cdef_class_rep object looks wrong to
> me.  Instead of copying the reference count, shouldn't the count for the
> new object be 1?  But I'm not sure I understand the way this code is
> supposed to work so I left it alone and provided a copy constructor for
> the refcount class.

I ended up pushing the following three changes:

   http://hg.savannah.gnu.org/hgweb/octave/rev/c98953e85220
   http://hg.savannah.gnu.org/hgweb/octave/rev/c23aee2104de
   http://hg.savannah.gnu.org/hgweb/octave/rev/396996f1dad0

Now all reference counts use octave_idx_type and I was able to eliminate
the object_count member variable in the cdef_class_rep object.  It was
not a reference count and was set but not used otherwise.

Until we have the C++20 atomic_ref class (or some interim replacement
for it), then I think the best we can do is to use the C11
atomic_fecth_add and atomic_fetch_sub functions in the dim_vector class.

jwe


Reply | Threaded
Open this post in threaded view
|

Re: Switch to std::atomic?

Dmitri A. Sergatskov
On Thu, Sep 26, 2019 at 2:29 PM John W. Eaton <[hidden email]> wrote:

>
> On 9/25/19 6:10 PM, John W. Eaton wrote:
> > On 9/25/19 10:34 AM, John W. Eaton wrote:
> >> On 9/13/19 5:29 PM, Rik wrote:
> >>
> >>> Would it make sense to switch to using std::atomic<octave_idx_type>?  In
> >>> that case, Octave would be portable to any machine with a valid C++
> >>> runtime
> >>> library, and we certainly require that.  The std::atomic class overloads
> >>> the (pre-, post-)(increment, decrement) operators so one could write
> >>> more
> >>> readable code with count++ rather than
> >>> OCTAVE_ATOMIC_POST_INCREMENT(count).
> >>
> >> Yes, now that it is a standard C++ feature, we should be using that.
> >> For a simple transition, can we first define our refcount class using
> >> std::atomic<T>?
> >>
> >> Would you like to make this change?  If not, I can look at it.
> >
> > I was curious to see what it would take and came up with the following
> > change.  I think this is OK as far as it goes, but I haven't pushed it.
> >
> > Notes and questions:
> >
> >    * The C11 atomic_fetch_add and atomic_fetch_sub functions were
> > required to handle the tricky way that the dim_vector class stores the
> > reference count.  I believe that to do this job in C++ requires the
> > atomic_ref object that is a new feature in C++20.  Either that or use a
> > separate reference count variable instead of storing it in the array of
> > values, but then as the comment in dim-vector.h says, we will need two
> > allocations, one for the rep object and one for the array that it will
> > then contain.
> >
> >    * Some reference counts use int and some use octave_idx_type.  Maybe
> > we should always use the same type?  Should it be int or octave_idx_type
> > or size_t?
> >
> >    * The copy constructor for the cdef_class_rep object looks wrong to
> > me.  Instead of copying the reference count, shouldn't the count for the
> > new object be 1?  But I'm not sure I understand the way this code is
> > supposed to work so I left it alone and provided a copy constructor for
> > the refcount class.
>
> I ended up pushing the following three changes:
>
>    http://hg.savannah.gnu.org/hgweb/octave/rev/c98953e85220
>    http://hg.savannah.gnu.org/hgweb/octave/rev/c23aee2104de
>    http://hg.savannah.gnu.org/hgweb/octave/rev/396996f1dad0
>
> Now all reference counts use octave_idx_type and I was able to eliminate
> the object_count member variable in the cdef_class_rep object.  It was
> not a reference count and was set but not used otherwise.
>
> Until we have the C++20 atomic_ref class (or some interim replacement
> for it), then I think the best we can do is to use the C11
> atomic_fecth_add and atomic_fetch_sub functions in the dim_vector class.
>
> jwe
>
>

There are some errors on clang compiles, e.g.:

http://buildbot.octave.org:8010/#/builders/9/builds/1121



../src/liboctave/util/oct-atomic.c:34:3: error: address argument to
atomic operation must be a pointer to _Atomic type ('octave_idx_type
*' (aka 'long *') invalid)
  atomic_fetch_add (x, 1);
  ^                 ~
/usr/lib64/clang/8.0.0/include/stdatomic.h:146:43: note: expanded from
macro 'atomic_fetch_add'
#define atomic_fetch_add(object, operand)
__c11_atomic_fetch_add(object, operand, __ATOMIC_SEQ_CST)
                                          ^                      ~~~~~~
../src/liboctave/util/oct-atomic.c:42:3: error: address argument to
atomic operation must be a pointer to _Atomic type ('octave_idx_type
*' (aka 'long *') invalid)
  atomic_fetch_sub (x, 1);
  ^                 ~
/usr/lib64/clang/8.0.0/include/stdatomic.h:149:43: note: expanded from
macro 'atomic_fetch_sub'
#define atomic_fetch_sub(object, operand)
__c11_atomic_fetch_sub(object, operand, __ATOMIC_SEQ_CST)
                                          ^                      ~~~~~~
2 errors generated.
make[2]: *** [Makefile:16937: liboctave/util/libutil_la-oct-atomic.lo] Error 1


Dmitri.
--

Reply | Threaded
Open this post in threaded view
|

Re: Switch to std::atomic?

Rik-4
In reply to this post by John W. Eaton
On 09/26/2019 12:01 PM, John W. Eaton wrote:

> On 9/25/19 6:10 PM, John W. Eaton wrote:
>> On 9/25/19 10:34 AM, John W. Eaton wrote:
>>> On 9/13/19 5:29 PM, Rik wrote:
>>>
>>>> Would it make sense to switch to using std::atomic<octave_idx_type>?  In
>>>> that case, Octave would be portable to any machine with a valid C++
>>>> runtime
>>>> library, and we certainly require that.  The std::atomic class overloads
>>>> the (pre-, post-)(increment, decrement) operators so one could write more
>>>> readable code with count++ rather than
>>>> OCTAVE_ATOMIC_POST_INCREMENT(count).
>>>
>>> Yes, now that it is a standard C++ feature, we should be using that.
>>> For a simple transition, can we first define our refcount class using
>>> std::atomic<T>?
>>>
>>> Would you like to make this change?  If not, I can look at it.
>>
>> I was curious to see what it would take and came up with the following
>> change.  I think this is OK as far as it goes, but I haven't pushed it.
>>
>> Notes and questions:
>>
>>    * The C11 atomic_fetch_add and atomic_fetch_sub functions were
>> required to handle the tricky way that the dim_vector class stores the
>> reference count.  I believe that to do this job in C++ requires the
>> atomic_ref object that is a new feature in C++20.  Either that or use a
>> separate reference count variable instead of storing it in the array of
>> values, but then as the comment in dim-vector.h says, we will need two
>> allocations, one for the rep object and one for the array that it will
>> then contain.
>>
>>    * Some reference counts use int and some use octave_idx_type.  Maybe
>> we should always use the same type?  Should it be int or octave_idx_type
>> or size_t?
>>
>>    * The copy constructor for the cdef_class_rep object looks wrong to
>> me.  Instead of copying the reference count, shouldn't the count for the
>> new object be 1?  But I'm not sure I understand the way this code is
>> supposed to work so I left it alone and provided a copy constructor for
>> the refcount class.
>
> I ended up pushing the following three changes:
>
>   http://hg.savannah.gnu.org/hgweb/octave/rev/c98953e85220
>   http://hg.savannah.gnu.org/hgweb/octave/rev/c23aee2104de
>   http://hg.savannah.gnu.org/hgweb/octave/rev/396996f1dad0
>
> Now all reference counts use octave_idx_type and I was able to eliminate
> the object_count member variable in the cdef_class_rep object.  It was
> not a reference count and was set but not used otherwise.
>
> Until we have the C++20 atomic_ref class (or some interim replacement for
> it), then I think the best we can do is to use the C11 atomic_fecth_add
> and atomic_fetch_sub functions in the dim_vector class.
>
I like it.  This seems much cleaner.  The benchmark for bm.toeplitz.orig.m
shifted from 7.5 to 7.6 seconds.  That is within the noise for this
measurement so no problems there.  There doesn't seem to be any ill
effects.  Running the GUI on an installed version and using
__run_test_suite__ produced no errors nor any segfaults.

It's done, so maybe it doesn't matter, but I very much doubt that objects
are shared so extensively that they require octave_idx_type-sized
refcounts.  On my machine, 'int' is 4 bytes while octave_idx_type is 8
bytes.  It's not the extra 4B of storage that are important, but maybe the
fact that 'int' is supposed to always be the most natural unit for the
machine in question.  Maybe there are slight performance benefits to using
int for reference counts?

Was there a particular reason to make oct-atomic.c a 'C' language file
rather than a C++ file?  If possible, it would seem better to make this C++
and possible also add an 'inline' declaration so that we don't get function
call overhead.

--Rik

Reply | Threaded
Open this post in threaded view
|

Re: Switch to std::atomic?

John W. Eaton
Administrator
On 9/26/19 5:18 PM, Rik wrote:

> Was there a particular reason to make oct-atomic.c a 'C' language file
> rather than a C++ file?  If possible, it would seem better to make this C++
> and possible also add an 'inline' declaration so that we don't get function
> call overhead.

To try to inline the calls to the atomic_fetch_add and atomic_fetch_sub,
we would need to include stdatomic.h in dim_vector.h.  I didn't think
that was a good idea as it could conflict with other C++ header files.

When I tried to include stdatomic.h in oct-refcount.h and use those
functions to provide direct replacements for the old OCTAVE_ATOMIC_*
macros, I ran into errors.  Testing now, I get errors when using g++ to
compile a simple C++ file that has only

   #include <stdatomic.h>

in it.  So it seems best to me to just use it from C and provide
wrappers that can be used in C++.

jwe

Reply | Threaded
Open this post in threaded view
|

Re: Switch to std::atomic?

Rik-4
On 09/26/2019 02:43 PM, John W. Eaton wrote:

> On 9/26/19 5:18 PM, Rik wrote:
>
>> Was there a particular reason to make oct-atomic.c a 'C' language file
>> rather than a C++ file?  If possible, it would seem better to make this C++
>> and possible also add an 'inline' declaration so that we don't get function
>> call overhead.
>
> To try to inline the calls to the atomic_fetch_add and atomic_fetch_sub,
> we would need to include stdatomic.h in dim_vector.h.  I didn't think
> that was a good idea as it could conflict with other C++ header files.
>
> When I tried to include stdatomic.h in oct-refcount.h and use those
> functions to provide direct replacements for the old OCTAVE_ATOMIC_*
> macros, I ran into errors.  Testing now, I get errors when using g++ to
> compile a simple C++ file that has only
>
>   #include <stdatomic.h>
>
> in it.  So it seems best to me to just use it from C and provide wrappers
> that can be used in C++.
>

Maybe my misinterpretation, but I thought to include the C++ header file
<atomic>, rather than the C header file <stdatomic.h>.  I was looking at
this reference: http://www.cplusplus.com/reference/atomic/ which seems to
show that atomic_fetch_add exists under C++, but that the prototype we need
to use may require casting to volatile.

However, I couldn't get it to compile after making those changes, so it is
harder than I think.

--Rik


Reply | Threaded
Open this post in threaded view
|

Re: Switch to std::atomic?

John W. Eaton
Administrator
On 9/26/19 9:53 PM, Rik wrote:

> Maybe my misinterpretation, but I thought to include the C++ header file
> <atomic>, rather than the C header file <stdatomic.h>.  I was looking at
> this reference: http://www.cplusplus.com/reference/atomic/ which seems to
> show that atomic_fetch_add exists under C++, but that the prototype we need
> to use may require casting to volatile.
>
> However, I couldn't get it to compile after making those changes, so it is
> harder than I think.

For

#include <atomic>

int
main (void)
{
   int x = 1;
   volatile int *xp = &x;
   std::atomic_fetch_add (xp, 1);
   return 0;
}

G++ issues the following error:

atom.cc: In function ‘int main()’:
atom.cc:8:31: error: no matching function for call to
‘atomic_fetch_add(volatile int*&, int)’
     8 |   std::atomic_fetch_add (xp, 1);
       |                               ^
In file included from atom.cc:1:
/usr/include/c++/9/atomic:1417:5: note: candidate: ‘template<class _ITp>
_ITp std::atomic_fetch_add(std::atomic<_ITp>*, std::__atomic_diff_t<_ITp>)’
  1417 |     atomic_fetch_add(atomic<_ITp>* __a,
       |     ^~~~~~~~~~~~~~~~
/usr/include/c++/9/atomic:1417:5: note:   template argument
deduction/substitution failed:
atom.cc:8:31: note:   mismatched types ‘std::atomic<_ITp>’ and ‘volatile
int’
     8 |   std::atomic_fetch_add (xp, 1);
       |                               ^
In file included from atom.cc:1:
/usr/include/c++/9/atomic:1423:5: note: candidate: ‘template<class _ITp>
_ITp std::atomic_fetch_add(volatile std::atomic<_ITp>*,
std::__atomic_diff_t<_ITp>)’
  1423 |     atomic_fetch_add(volatile atomic<_ITp>* __a,
       |     ^~~~~~~~~~~~~~~~
/usr/include/c++/9/atomic:1423:5: note:   template argument
deduction/substitution failed:
atom.cc:8:31: note:   mismatched types ‘volatile std::atomic<_ITp>’ and
‘volatile int’
     8 |   std::atomic_fetch_add (xp, 1);
       |
                   ^

The candidate functions both accept std::atomic objects, not bare
pointers.  That's consistent with the prototypes shown here:

   https://en.cppreference.com/w/cpp/atomic/atomic_fetch_add

It looks like the C interface is not available in C++.  Is the info
available at cplusplus.com out of date?

jwe

Reply | Threaded
Open this post in threaded view
|

Re: Switch to std::atomic?

John W. Eaton
Administrator
In reply to this post by Dmitri A. Sergatskov
On 9/26/19 4:25 PM, Dmitri A. Sergatskov wrote:

> There are some errors on clang compiles, e.g.:
>
> http://buildbot.octave.org:8010/#/builders/9/builds/1121
>
>
>
> ../src/liboctave/util/oct-atomic.c:34:3: error: address argument to
> atomic operation must be a pointer to _Atomic type ('octave_idx_type
> *' (aka 'long *') invalid)
>    atomic_fetch_add (x, 1);
>    ^                 ~
> /usr/lib64/clang/8.0.0/include/stdatomic.h:146:43: note: expanded from
> macro 'atomic_fetch_add'
> #define atomic_fetch_add(object, operand)
> __c11_atomic_fetch_add(object, operand, __ATOMIC_SEQ_CST)
>                                            ^                      ~~~~~~
> ../src/liboctave/util/oct-atomic.c:42:3: error: address argument to
> atomic operation must be a pointer to _Atomic type ('octave_idx_type
> *' (aka 'long *') invalid)
>    atomic_fetch_sub (x, 1);
>    ^                 ~
> /usr/lib64/clang/8.0.0/include/stdatomic.h:149:43: note: expanded from
> macro 'atomic_fetch_sub'
> #define atomic_fetch_sub(object, operand)
> __c11_atomic_fetch_sub(object, operand, __ATOMIC_SEQ_CST)
>                                            ^                      ~~~~~~
> 2 errors generated.
> make[2]: *** [Makefile:16937: liboctave/util/libutil_la-oct-atomic.lo] Error 1
>
>
> Dmitri.
> --
>

Is the following change OK or is this a dangerous or invalid cast?

--- a/liboctave/util/oct-atomic.c
+++ b/liboctave/util/oct-atomic.c
@@ -31,7 +31,7 @@ along with Octave; see the file COPYING.
  octave_idx_type
  octave_atomic_increment (octave_idx_type *x)
  {
-  atomic_fetch_add (x, 1);
+  atomic_fetch_add ((_Atomic octave_idx_type *) x, 1);

    return *x;
  }
@@ -39,7 +39,7 @@ octave_atomic_increment (octave_idx_type
  octave_idx_type
  octave_atomic_decrement (octave_idx_type *x)
  {
-  atomic_fetch_sub (x, 1);
+  atomic_fetch_sub ((_Atomic octave_idx_type *) x, 1);

    return *x;
  }

jwe

Reply | Threaded
Open this post in threaded view
|

Re: Is fork() broken in octave 5.1 ?

Dr. K. nick
In reply to this post by John W. Eaton
On 9/26/19 8:25 PM, John W. Eaton wrote:

> On 9/26/19 1:42 PM, Mike Miller wrote:
>> On Thu, Sep 26, 2019 at 19:27:55 +0200, Kay Nick wrote:
>>> Nevertheless I find this behavior not intuitive... What is the reason
>>> for preventing fork() from being used in a simple script?
>>
>> It is related to this (still open) bug report:
>> https://savannah.gnu.org/bugs/?45625
>
> Yes, the check was added to prevent simple calls to fork at the
> command line from creating two copies of Octave both competing for
> input from the terminal and possibly confusing users.  Once this
> situation occurs, it can be difficult to recover from without killing
> the session.
>
> The check that was added doesn't distinguish between the command line
> and a script evaluated from the command line.  A script executes in
> the scope of the caller and the check was for execution at the
> top-level scope rather than checking the depth of the call stack.
>
> Maybe we could change the condition so that it will allow fork if it
> is executed from a script but not directly from the command line.
>
> jwe
>

> Maybe we could change the condition so that it will allow fork if it
> is executed from a script but not directly from the command line.
This seems a reasonable thing to do as it would unbreak many scripts
that used to work with octave 3.8 and fail with 5.1. 

> It is related to this (still open) bug report:
> https://savannah.gnu.org/bugs/?45625 
Mike, thanks for that hint. I find it very helpful but seem to have
missed it when searching ...

Marcin

P. S. Is there some (new?) convention to add a reply beneath an
email/comment/statement it is referring to instead of above (as it used
to be?) ?



Reply | Threaded
Open this post in threaded view
|

Re: Switch to std::atomic?

Pantxo
In reply to this post by John W. Eaton
John W. Eaton wrote
> On 9/25/19 6:10 PM, John W. Eaton wrote:
>
> I ended up pushing the following three changes:
>
>    http://hg.savannah.gnu.org/hgweb/octave/rev/c98953e85220
>    http://hg.savannah.gnu.org/hgweb/octave/rev/c23aee2104de
>    http://hg.savannah.gnu.org/hgweb/octave/rev/396996f1dad0

I am unable to compile Octave with gcc 6.3 (the default in debian stretch)
and OpenMP. I get a bunch of error that look as follows:

In file included from ../octave/liboctave/util/oct-atomic.c:29:0:
/usr/lib/gcc/x86_64-linux-gnu/6/include/stdatomic.h:40:1: sorry,
unimplemented: ‘_Atomic’ with OpenMP
 typedef _Atomic _Bool atomic_bool;

This seems to have been fixed in later varsions of gcc, see

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65467

Pantxo



--
Sent from: https://octave.1599824.n4.nabble.com/Octave-Maintainers-f1638794.html

Reply | Threaded
Open this post in threaded view
|

Re: Switch to std::atomic?

John W. Eaton
Administrator
On 9/27/19 3:48 AM, Pantxo wrote:

> John W. Eaton wrote
>> On 9/25/19 6:10 PM, John W. Eaton wrote:
>>
>> I ended up pushing the following three changes:
>>
>>     http://hg.savannah.gnu.org/hgweb/octave/rev/c98953e85220
>>     http://hg.savannah.gnu.org/hgweb/octave/rev/c23aee2104de
>>     http://hg.savannah.gnu.org/hgweb/octave/rev/396996f1dad0
>
> I am unable to compile Octave with gcc 6.3 (the default in debian stretch)
> and OpenMP. I get a bunch of error that look as follows:
>
> In file included from ../octave/liboctave/util/oct-atomic.c:29:0:
> /usr/lib/gcc/x86_64-linux-gnu/6/include/stdatomic.h:40:1: sorry,
> unimplemented: ‘_Atomic’ with OpenMP
>   typedef _Atomic _Bool atomic_bool;
>
> This seems to have been fixed in later varsions of gcc, see
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65467
Does the attached patch avoid the problem for you with GCC 6.3?  I'm
assuming HAVE_STDATOMIC_H will be defined only if it can be included
without error, and that will be a sufficient test to avoid it if the
compiler has the problem you show above.

jwe



diffs.txt (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Switch to std::atomic?

Pantxo


Le ven. 27 sept. 2019 à 18:02, John W. Eaton <[hidden email]> a écrit :
On 9/27/19 3:48 AM, Pantxo wrote:
> John W. Eaton wrote
>> On 9/25/19 6:10 PM, John W. Eaton wrote:
>>
>> I ended up pushing the following three changes:
>>
>>     http://hg.savannah.gnu.org/hgweb/octave/rev/c98953e85220
>>     http://hg.savannah.gnu.org/hgweb/octave/rev/c23aee2104de
>>     http://hg.savannah.gnu.org/hgweb/octave/rev/396996f1dad0
>
> I am unable to compile Octave with gcc 6.3 (the default in debian stretch)
> and OpenMP. I get a bunch of error that look as follows:
>
> In file included from ../octave/liboctave/util/oct-atomic.c:29:0:
> /usr/lib/gcc/x86_64-linux-gnu/6/include/stdatomic.h:40:1: sorry,
> unimplemented: ‘_Atomic’ with OpenMP
>   typedef _Atomic _Bool atomic_bool;
>
> This seems to have been fixed in later varsions of gcc, see
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65467

Does the attached patch avoid the problem for you with GCC 6.3?  I'm
assuming HAVE_STDATOMIC_H will be defined only if it can be included
without error, and that will be a sufficient test to avoid it if the
compiler has the problem you show above.

jwe


After cset b47705865de, I can build again with gcc 6.3.
Thanks.
Reply | Threaded
Open this post in threaded view
|

More weirdness with fork() in octave 5.1.0

Dr. K. nick
In reply to this post by John W. Eaton
cat wb_ftest.m
printf("start\n");
ftest();
printf("stop\n");

cat ftest.m
function ftest()
  [pid msg]=fork();
  if (pid > 0)
    disp("par start")
    pause(3);
    disp("par term")
  elseif (pid==0)
    disp("chl start")
    pause(1);
    disp("chl term")
  endif
endfunction

My procedure:

1. I start octave

ps -Af | grep octave  (checking process list before running wb_ftest.m)
nick     1807    1071  0 14:18 tty2     00:00:00 /usr/bin/octave --gui
nick     1808    1807 97 14:18 ?        00:00:10
/usr/lib/octave/5.1.0/exec/x86_64-pc-linux-gnu/octave-gui --gui
nick     1823    1487  0 14:18 pts/3    00:00:00 grep octave

2. I run wb_ftest.m from command line

>> wb_ftest
start
par start
chl start
chl term
stop
>> par term
stop
>>

ps -Af | grep octave (checking process list wb_ftest.m has finished)
nick     1807    1071  0 14:18 tty2     00:00:00 /usr/bin/octave --gui
nick     1808    1807  7 14:18 ?        00:00:16
/usr/lib/octave/5.1.0/exec/x86_64-pc-linux-gnu/octave-gui --gui
nick     1863    1808  0 14:21 ?        00:00:00
/usr/lib/octave/5.1.0/exec/x86_64-pc-linux-gnu/octave-gui --gui
nick     1871    1487  0 14:22 pts/3    00:00:00 grep octave

As it seems, last line in wb_ftest.m (printf("stop\n"); ) is being
executed twice although the 'pid==0'-part in ftest.m should have
finished (I'l like it to have finished) by this time. Is this intended
or a bug? What would be the proper way to correct for this behavior?

The process listings before and after running this script show, that a
new process is being fork()-ed correctly but it won't be terminated
(although the parent stays around longer than the child) properly. Even
after closing octave (clicking on x) the forked process stays around.

ps -Af | grep octave
nick     1917       1  0 14:24 ?        00:00:00
/usr/lib/octave/5.1.0/exec/x86_64-pc-linux-gnu/octave-gui --gui
nick     1981    1487  0 14:31 pts/3    00:00:00 grep octave

The system is Arch Linux 64 Bit.

Marcin

12