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

 
 

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

Submitter:  None
Submitted:  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:  * dev
Operating System:  * GNU/Linux
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       

 

( Jump to the original submission )

Mon 03 Jan 2022 04:11:29 PM UTC, comment #10: 

It's probably too late to squeeze this in just before the release of Octave 7.
Re-setting to "dev".

Markus Mützel <mmuetzel>
Project Member
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 mmuetzel (Posted a comment)
  • -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)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only project members can vote.

     

     

     

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-01-03 mmuetzel Release7.0.90 dev
    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.9