bugGNU Octave - Bugs: bug #61370, gcc's thread sanitizer reports...

 
 

bug #61370: gcc's thread sanitizer reports several race conditions

Submitted by:  None
Submitted on:  Sat 23 Oct 2021 10:23:45 AM UTC  
 
Category:  Interpreter Severity:  4 - Important
Priority:  5 - Normal Item Group:  Unexpected Error or Warning
Status:  Need Info Assigned to:  None
Originator Name:  Reinhard Originator Email:  -email is unavailable-
Open/Closed:  Open Release:  7.0.90
Operating System:  GNU/Linux

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Thu 25 Nov 2021 04:15:16 AM UTC, comment #9: 

The patch with "std::atomic<int>" is a good candidate for inclusion into Octave 7.

@jwe: any objections?

Kai Torben Ohlhus <siko1056>
Project Member
Tue 26 Oct 2021 10:00:48 AM UTC, comment #8: 

Regarding your second point, the interface for C functions, we could either write C++ wrapper functions which could be called from C, or use non atomic variables which must be accessed through C-style atomic functions only.

Anonymous
Tue 26 Oct 2021 08:13:59 AM UTC, comment #7: 

Dear John W. Eaton,

sig_atomic_t is atomic just for normal signal handlers (e.g. using the signal function), but not for concurrent threads. In case of octave, the signal handler is executed in a concurrent thread. See the code below:

  if (pthread_create (&sighandler_thread_id, 0, signal_watcher, handler))
    {
      // FIXME: what else should we do?
      abort ();
    }

static void *
signal_watcher (void *arg)
{
 ...

  while (1)
    {
      int sig_caught;

      if (sigwait (async_signals, &sig_caught))
        {
          abort ();
        }

      (*handler) (sig_caught);
    }
}

But of course, octave_interrupt_state could be declared as std::atomic<int> as well.

comment #5:

> Most of this proposed change seems OK to me except I find the declaration
>

> std::atomic<sig_atomic_t> octave_interrupt_state;

>
> to be a bit strange since I thought sig_atomic_t was already supposed to be an atomic type.
>
> More importantly, the proposed change moves the octave_interrupt_state variable inside an #if defined (__cplusplus) / #endif block so that it is no longer declared for C programs.  The original intent was to allow quit.h to be included in C or C++ source files and for some of the symbols it declares to be available in C source files.  I'm not sure how important this feature really is, but is it necessary to prevent octave_interrupt_state from being available in C source files?
>
> If we do decide that octave_interrupt_state only needs to be available in C++ source files, then I think we can just declare it as std::atomic<int> instead of std::atomic<sig_atomic_t>.

Anonymous
Tue 26 Oct 2021 01:43:13 AM UTC, comment #6: 

@Reinhard, thanks for the clarifications in comment #4.

Kai Torben Ohlhus <siko1056>
Project Member
Mon 25 Oct 2021 08:01:34 PM UTC, comment #5: 

Most of this proposed change seems OK to me except I find the declaration

std::atomic<sig_atomic_t> octave_interrupt_state;

to be a bit strange since I thought sig_atomic_t was already supposed to be an atomic type.

More importantly, the proposed change moves the octave_interrupt_state variable inside an #if defined (__cplusplus) / #endif block so that it is no longer declared for C programs.  The original intent was to allow quit.h to be included in C or C++ source files and for some of the symbols it declares to be available in C source files.  I'm not sure how important this feature really is, but is it necessary to prevent octave_interrupt_state from being available in C source files?

If we do decide that octave_interrupt_state only needs to be available in C++ source files, then I think we can just declare it as std::atomic<int> instead of std::atomic<sig_atomic_t>.

John W. Eaton <jwe>
Project Administrator
Mon 25 Oct 2021 05:03:04 PM UTC, comment #4: 

Dear Kai,

I'm developing a FEM toolkit located on GitHub (https://github.com/octave-user/mboct-fem-pkg). Recently I implemented parallel assembly of matrices and I wanted to make sure, that the code is free of race conditions. For that purpose I wrote this patch (https://savannah.gnu.org/bugs/download.php?file_id=52147) to make it run without any warnings. Nevertheless, this patch should be useful even if you don't use that package (e.g. just type Ctrl+C or execute kill ...).

comment #3:

> Thank you Reinhard for updating the patch.
>
> I do not fully understand what is meant by "I was not able to test the package this patch was made for".  Can you clarify this?

Anonymous
Mon 25 Oct 2021 04:11:52 AM UTC, comment #3: 

Thank you Reinhard for updating the patch.

I do not fully understand what is meant by "I was not able to test the package this patch was made for".  Can you clarify this?

Kai Torben Ohlhus <siko1056>
Project Member
Sun 24 Oct 2021 06:19:17 PM UTC, comment #2: 

Dear Kai,

I have attached a patch for the default branch. With this patch octave passes the test suite and no race condition is reported for octave-cli when typing Ctrl+C. However I was not able to test the package this patch was made for, because octave crashed during installation.

Best regards,
Reinhard

(file #52148)

Anonymous
Sun 24 Oct 2021 12:35:06 PM UTC, comment #1: 

Thanks for the analysis and the patch 🙂👍

The patch wraps many singal related boolean variables into std::atomic constructs.

I think jwe also worked on replacing variable instance counters into std::atomic constructs.

In general a patch for the Octave default branch (aka Octave 7) would be preferable, as the development of Octave 6 is finished with the soon coming Octave 6.4 release.

Kai Torben Ohlhus <siko1056>
Project Member
Sat 23 Oct 2021 10:23:45 AM UTC, original submission:  

Dear GNU/Octave developers,

If I compile the stable branch of Octave using -fsanitize=thread, I get several warnings about race conditions.
For example if I configure octave using the following options

./configure --prefix=/opt/octave-tsan LDFLAGS=-Wl,-rpath=/usr/local/lib --with-blas=-L/usr/local/lib -lopenblas --with-lapack=-L/usr/local/lib -lopenblas CXXFLAGS=-g -fsanitize=thread -Wall -O2 -march=native CFLAGS=-march=native -Wall -O2 -g -fsanitize=thread FFLAGS=-march=native -Wall -O2 -g -fsanitize=thread FCFLAGS=-march=native -Wall -O2 -g -fsanitize=thread

and execute

src/octave-cli

and then type Ctrl+C several times, I already get some warnings about race conditions. I have attached a patch for the stable branch which fixes most of it, at least in case of octave-cli. I did not check octave-gui since it would be required to rebuild the Qt5 library with -fsanitize=thread.
So far, I have executed all tests and there are no regressions when applying the patch.
If you want, I can port those patch to the default branch.

Best regards,
Reinhard

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #52148:  octave-fix-race-conditions-default.patch added by None (11KiB - text/x-patch)
file #52147:  octave-fix-race-conditions.patch added by None (7KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by siko1056
  • -email is unavailable- added by None (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-11-25 siko1056 Release6.3.0 => 7.0.90
    2021-10-24 None Attached File- => Added octave-fix-race-conditions-default.patch, #52148
    2021-10-24 siko1056 Severity3 - Normal => 4 - Important
        StatusNone => Need Info
    2021-10-24 siko1056 Carbon-Copy- => Added jwe
    2021-10-23 None Attached File- => Added octave-fix-race-conditions.patch, #52147

    Back to the top


    Powered by Savane 3.7