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:  Fixed Assigned to:  None
Originator Name:  Reinhard Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * dev
Operating System:  * GNU/Linux Fixed Release:  None
Planned Release:  9.1.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 16 May 2023 03:50:00 AM UTC, comment #30: 

No response from jwe to comment #29 after a month.  Going ahead and closing report.

Rik <rik5>
Group administrator
Wed 12 Apr 2023 08:42:38 PM UTC, comment #29: 

@jwe: You made a comment that one particular piece of code in comment #26 should not be applied.  The rest of the patch was applied with no ill effects.  This bug is marked "Ready For Test" which it has passed.  Do you want to keep this report open to review the code in comment #26, or just let it be and we close this report.

Rik <rik5>
Group administrator
Tue 21 Feb 2023 11:18:46 PM UTC, comment #28: 

I have pushed "patchset_big.patch" to default (Octave 9) at https://hg.savannah.gnu.org/hgweb/octave/rev/5f0b8101234e under Reinhard's name.

As described in the previous comment, this is everything except for the code in comment #26, which needs someone to examine it and say whether it needs to be fixed or deleted.

With TSAN, the patched code gives fewer warnings than the unpatched one.

Please test this patchset. Will wait for CI.

Arun Giridhar <arungiridhar>
Group Member
Fri 17 Feb 2023 04:13:45 PM UTC, comment #27: 

I've updated these patches to work against current default (hg id 45f8b601c992) and split them into two sets:

  • patchset_big.patch is everything except the code mentioned in comment #26. The commit message is in big.txt.


  • patchset_small.patch is only the code mentioned in comment #26. The commit message is in small.txt.


The patches can be imported in any order. Building the small patch will require the big patch to be applied too, but they are independent other than that.

(file #54370, file #54371, file #54372, file #54373)

Arun Giridhar <arungiridhar>
Group Member
Sat 03 Dec 2022 08:09:59 PM UTC, comment #26: 

I backed out the changeset.  We can probably apply it on default, but first I want to understand how these two bits of code are equivalent.

Current:


  std::cerr << "warning: broken pipe" << std::endl;

  // Don't loop forever on account of this.
  // FIXME: is this really needed?  Does it do anything
  // useful now?

  if (pipe_handler_error_count++ > 100
      && octave_interrupt_state >= 0)
    octave_interrupt_state++;


Patched:


  std::cerr << "warning: broken pipe" << std::endl;

  // Don't loop forever on account of this.
  // FIXME: is this really needed?  Does it do anything
  // useful now?

  const int curr_pipe_handler_error_count = pipe_handler_error_count++;

  if (curr_pipe_handler_error_count > 100)
    {
      sig_atomic_t curr_interrupt_state
      = octave_interrupt_state.load();

      sig_atomic_t new_interrupt_state;

      do
        new_interrupt_state = curr_interrupt_state + 1;
      while (curr_interrupt_state >= 0 &&
         ! octave_interrupt_state.compare_exchange_weak
         (curr_interrupt_state, new_interrupt_state));
    }


The original intent was to not get stuck continually responding to a long series of SIGPIPE signals.  If I remember correctly, that was sometimes a problem long ago when piping output to the pager or to gnuplot.  I don't know whether it is a real problem now.  Maybe this code could just be removed.  Keeping it probably does no harm, but the new code should be equivalent to the old.  I don't understand the do-while part of the new code.  Why is that needed here?  Previously there was no loop, just a conditional so that the interrupt_state variable was incremented IF it was already >= 0 (not already responding to an interrupt) and the pipe error count was > 100 (because we've seen a long series of SIGPIPE signals).

John W. Eaton <jwe>
Group administrator
Sat 03 Dec 2022 04:23:59 PM UTC, comment #25: 

I don't agree that these changes were appropriate for stable at this point in the release process.

John W. Eaton <jwe>
Group administrator
Sat 03 Dec 2022 04:07:39 PM UTC, comment #24: 

Thanks. Pushed to https://hg.savannah.gnu.org/hgweb/octave/rev/d9970470108a

I saw that your name is already listed in the contributors file, so I didn't add it afresh.

Marking as Ready For Test.

Arun Giridhar <arungiridhar>
Group Member
Sat 03 Dec 2022 03:12:13 PM UTC, comment #23: 

You can use

Reinhard Resch r_resch@a1.net


comment #22:

> @Reinhard, could you also state how you want to be credited pls?
>
> Currently, I have this:


> Reinhard <octave-user@a1.net>


>

Anonymous
Fri 02 Dec 2022 06:45:24 PM UTC, comment #22: 

@Reinhard, could you also state how you want to be credited pls?

Currently, I have this:

Reinhard <octave-user@a1.net>


Arun Giridhar <arungiridhar>
Group Member
Fri 02 Dec 2022 06:42:17 PM UTC, comment #21: 

Updated patches attached addressing the first point from comment #17.

Retargeting to 8.1.0 (stable).

(file #54049, file #54050)

Arun Giridhar <arungiridhar>
Group Member
Fri 02 Dec 2022 06:23:28 PM UTC, comment #20: 

There are probably many issues with race conditions in Octave. Afaict, jwe is currently working on some of them related to the GUI settings.
One fix will probably not fix all of the issues. But we need to start somewhere.
Imho, this patch is a step in the right direction. But it's just one step (of many more to hopefully come).

Markus Mützel <mmuetzel>
Group administrator
Fri 02 Dec 2022 06:17:05 PM UTC, comment #19: 

No change. I am checking that I applied the patch correctly, but so
far I do not see obvious improvements or degradation with basic tests
like start/quit octave; start, "plot(1:3)", quit; etc...

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Fri 02 Dec 2022 06:13:24 PM UTC, comment #18: 

@Dmitri: Is this a regression with the patch from here?

Markus Mützel <mmuetzel>
Group administrator
Fri 02 Dec 2022 06:12:27 PM UTC, comment #17: 

Reading through the thread, there are two points which I couldn't see if they are addressed in the updated patch:
- `extern OCTAVE_API std::atomic<sig_atomic_t> octave_interrupt_state;` should be replaced by `extern OCTAVE_API std::atomic<int> octave_interrupt_state;`. That seems to have been agreed on.
- `quit.h` in a public header. Is it ok to move octave_interrupt_state to a block where it is no longer declared for C programs?

Wrt the second point: A Google search for `octave_interrupt_state` didn't result in a single hit in a C source file. That doesn't necessarily mean that there isn't code out the that uses it in C. But I'd guess that is pretty unlikely.

From my point of view, this could go on stable when the first point is fixed.

Markus Mützel <mmuetzel>
Group administrator
Fri 02 Dec 2022 06:09:20 PM UTC, comment #16: 

I do not see much improvements. For one thing, with TSAN, I cannot even start octave on gui mode (./run-octave --gui). In clang compiled version I just got
a black screen (and two octave processes running which I have to kill -9).
With gcc compiled I got the attached windows with a popup that says
("GNU Octave" is not responding. Force Quit / Wait) and it is killed with -9 as well.




Dmitri A. Sergatskov <dasergatskov>
Fri 02 Dec 2022 05:43:42 PM UTC, comment #15: 

To quantify this, I built Octave with these settings:


CFLAGS="-pipe -fsanitize=thread" CXXFLAGS=$CFLAGS FFLAGS=$CFLAGS ../configure --disable-docs


Then I started `./run_octave`, waited for the initial set of thread warnings to stop, hit Ctrl-C, and then exited.

Without the patch, it gives me 8+4 = 12 warnings, with 8 on startup and 4 on Ctrl-C.

With the patch, it gives me 8+0 = 8 warnings, the final 4 on Ctrl-C being eliminated.

(Same results for both stable and default.)

Arun Giridhar <arungiridhar>
Group Member
Fri 02 Dec 2022 02:48:24 PM UTC, comment #14: 

I was able to update the patch against both stable (Octave 8) and default (Octave 9). They build and pass "make check". Updated patches attached.

Please review the control flow logic.

Maintainers: What are your thoughts on including these fixes for Octave 8?

(file #54046, file #54047)

Arun Giridhar <arungiridhar>
Group Member
Wed 30 Nov 2022 06:08:32 PM UTC, comment #13: 

Marking this as targeted for current default (to be released as Octave 9). @Reinhard, if you could, please prepare an updated patch against the current dev sources (Octave 9.0.0). If it's too difficult, I can apply what you have and see what doesn't apply.

There are several races in the code. This would be a good start to eliminating them.

Arun Giridhar <arungiridhar>
Group Member
Wed 19 Oct 2022 06:54:54 PM UTC, comment #12: 

Dear Arun Giridhar,

I did not check with newer versions of Octave. So far I'm still using the same build of Octave 6.4.0 with the patch applied. But if you are willing to apply the patch to Octave 8, then of course I can port it to that version.

Anonymous
Wed 19 Oct 2022 06:14:30 PM UTC, comment #11: 

Checking in on this bug report. The patch no longer applies cleanly to the current tree (stable and default both). Is this still a bug or has it been solved with a different technique? Should the patch be updated for Octave 8?

Arun Giridhar <arungiridhar>
Group Member
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>
Group administrator
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>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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 #54370:  patchset_big.patch added by arungiridhar (9KiB - text/x-patch)
file #54371:  patchset_small.patch added by arungiridhar (1KiB - text/x-patch)
file #54372:  big.txt added by arungiridhar (1KiB - text/plain)
file #54373:  small.txt added by arungiridhar (196B - text/plain)
file #54048:  octave8_tsan.png added by dasergatskov (111KiB - image/png)
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 rik5 (Posted a comment)
  • -email is unavailable- added by dasergatskov (Updated the item)
  • -email is unavailable- added by arungiridhar (Posted a comment)
  • -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 group members can vote.

     

    Follow 22 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-05-16 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2023-02-21 arungiridhar Planned Release8.1.0 9.1.0
    2023-02-17 arungiridhar Attached File- Added patchset_big.patch, #54370
        Attached File- Added patchset_small.patch, #54371
        Attached File- Added big.txt, #54372
        Attached File- Added small.txt, #54373
    2022-12-03 arungiridhar StatusNeed Info Ready For Test
    2022-12-02 arungiridhar Attached File- Added racefixes_against_default.patch, #54049
        Attached File- Added racefixes_against_stable.patch, #54050
        Planned Release9.1.0 8.1.0
    2022-12-02 dasergatskov Attached File- Added octave8_tsan.png, #54048
    2022-12-02 arungiridhar Attached File- Added racefixes_against_default.patch, #54046
        Attached File- Added racefixes_against_stable.patch, #54047
    2022-11-30 arungiridhar Planned ReleaseNone 9.1.0
    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.13-4448.
    Corresponding source code