Fix bug in dataflow <-> GUI progress communication (race condition, thread blocking)

joern274 · joern274 · commit 36378a46f822 · 2026-04-22T11:47:48.000+02:00
diff --git a/plugins/dataflow_analysis/include/dataflow_analysis/utils/progress_printer.h b/plugins/dataflow_analysis/include/dataflow_analysis/utils/progress_printer.h
@@ -36,7 +36,12 @@ namespace hal
         public:
             ProgressPrinter(u32 max_message_size = 0);
 
-            void print_progress(float progress, const std::string& message = "");
+            void print_progress_to_stderr(float progress, const std::string& message = "");
+
+            // GUI calls must not be called from semaphore/mutex protected scope, otherwise threads might block each other
+            void print_progress_to_gui(int percent = -1); // use m_last_percentage if negative
+
+            void print_message_to_gui(const std::string& message);
 
             void clear();
 
@@ -48,8 +53,10 @@ namespace hal
 
             u32 m_printed_progress;
             std::string m_last_message;
+            std::string m_gui_message; // not all messages are relevant for GUI
             u32 m_bar_width;
             u32 m_max_message_size;
+            int m_last_percentage;
             int m_terminal_width;    // no terminal found if negative
         };
     }    // namespace dataflow
diff --git a/plugins/dataflow_analysis/src/evaluation/evaluation.cpp b/plugins/dataflow_analysis/src/evaluation/evaluation.cpp
@@ -128,9 +128,11 @@ namespace hal
                     // scan groups until all or done
                     float original_size = sorted_results.size();
                     ProgressPrinter progress_bar;
+                    progress_bar.print_message_to_gui("dataflow: evaluate results …");
                     while (!sorted_results.empty())
                     {
-                        progress_bar.print_progress((original_size - sorted_results.size()) / original_size);
+                        progress_bar.print_progress_to_stderr((original_size - sorted_results.size()) / original_size);
+                        progress_bar.print_progress_to_gui();
 
                         // precompute the group indices of each gate
                         std::unordered_map<u32, u32> max_group_size_of_gate;
diff --git a/plugins/dataflow_analysis/src/pre_processing/pre_processing.cpp b/plugins/dataflow_analysis/src/pre_processing/pre_processing.cpp
@@ -88,6 +88,8 @@ namespace hal
                     measure_block_time("identifying successors and predecessors of sequential gates") ProgressPrinter progress_bar;
                     float cnt = 0;
 
+                    progress_bar.print_message_to_gui("dataflow: preprocessing …");
+
                     // cache map of nets to group indices of known net groups
                     std::unordered_map<const Net*, u32> net_to_group_index;
                     for (u32 i = 0; i < config.known_net_groups.size(); i++)
@@ -110,7 +112,8 @@ namespace hal
                     for (const auto& gate : netlist_abstr.target_gates)
                     {
                         cnt++;
-                        progress_bar.print_progress(cnt / netlist_abstr.target_gates.size());
+                        progress_bar.print_progress_to_stderr(cnt / netlist_abstr.target_gates.size());
+                        progress_bar.print_progress_to_gui();
 
                         // create sets even if there are no successors
                         if (netlist_abstr.gate_to_successors.find(gate->get_id()) == netlist_abstr.gate_to_successors.end())
diff --git a/plugins/dataflow_analysis/src/pre_processing/register_stage_identification.cpp b/plugins/dataflow_analysis/src/pre_processing/register_stage_identification.cpp
@@ -53,7 +53,7 @@ namespace hal
                         for (const auto& sequential_gate : netlist_abstr.target_gates)
                         {
                             cnt++;
-                            progress_bar.print_progress(cnt / netlist_abstr.target_gates.size());
+                            progress_bar.print_progress_to_stderr(cnt / netlist_abstr.target_gates.size());
 
                             auto current = sequential_gate->get_id();
 
@@ -140,7 +140,7 @@ namespace hal
                         for (u32 i = 0; i < stages_to_split; ++i)
                         {
                             ++cnt;
-                            progress_bar.print_progress(cnt / stages_to_split);
+                            progress_bar.print_progress_to_stderr(cnt / stages_to_split);
 
                             std::unordered_map<u32, std::vector<u32>> move_out_reasons;
                             for (auto g : ctx.stages[i])
@@ -238,7 +238,7 @@ namespace hal
                         for (const auto& stage : directional_stages[i].stages)
                         {
                             cnt++;
-                            progress_bar.print_progress(cnt / (directional_stages[0].stages.size() + directional_stages[1].stages.size()));
+                            progress_bar.print_progress_to_stderr(cnt / (directional_stages[0].stages.size() + directional_stages[1].stages.size()));
                             for (auto it = directional_stages[1 - i].stages.begin(); it != directional_stages[1 - i].stages.end();)
                             {
                                 auto& other_stage = *it;
@@ -296,7 +296,7 @@ namespace hal
                     std::set<std::vector<u32>> seen;
                     for (auto& [g, stages] : netlist_abstr.gate_to_register_stages)
                     {
-                        progress_bar.print_progress(++cnt / netlist_abstr.gate_to_register_stages.size());
+                        progress_bar.print_progress_to_stderr(++cnt / netlist_abstr.gate_to_register_stages.size());
 
                         if (stages.size() > 1)
                         {
@@ -336,7 +336,7 @@ namespace hal
                     cnt = 0;
                     for (const auto& combine : spread_stages)
                     {
-                        progress_bar.print_progress(++cnt / netlist_abstr.gate_to_register_stages.size());
+                        progress_bar.print_progress_to_stderr(++cnt / netlist_abstr.gate_to_register_stages.size());
 
                         for (auto stage_id : combine)
                         {
diff --git a/plugins/dataflow_analysis/src/processing/processing.cpp b/plugins/dataflow_analysis/src/processing/processing.cpp
@@ -62,6 +62,7 @@ namespace hal
                             ctx.pass_counter = end_id;
                             if (ctx.pass_counter >= num_passes)
                             {
+                                std::lock_guard guard(ctx.result_mutex);
                                 ctx.done = true;
                             }
                         }
@@ -72,13 +73,16 @@ namespace hal
 
                             if (auto it = ctx.pass_outcome.find({current_state, current_pass.id}); it != ctx.pass_outcome.end())
                             {
-                                // early exit, outcome is already known
-                                std::lock_guard guard(ctx.result_mutex);
-                                ctx.new_recurring_results.emplace_back(current_state, current_pass.id, it->second);
-                                ctx.finished_passes++;
-                                m_progress_printer.print_progress((float)ctx.finished_passes / ctx.current_passes.size(),
+                                {
+                                    // early exit, outcome is already known
+                                    std::lock_guard guard(ctx.result_mutex);
+                                    ctx.new_recurring_results.emplace_back(current_state, current_pass.id, it->second);
+                                    ctx.finished_passes++;
+                                    m_progress_printer.print_progress_to_stderr((float)ctx.finished_passes / ctx.current_passes.size(),
                                                                   std::to_string(ctx.finished_passes) + "\\" + std::to_string(ctx.current_passes.size()) + " ("
                                                                       + std::to_string(ctx.new_unique_groupings.size()) + " new results)");
+                                }
+                                m_progress_printer.print_progress_to_gui();
                                 continue;
                             }
 
@@ -107,10 +111,11 @@ namespace hal
                                 }
 
                                 ctx.finished_passes++;
-                                m_progress_printer.print_progress((float)ctx.finished_passes / ctx.current_passes.size(),
+                                m_progress_printer.print_progress_to_stderr((float)ctx.finished_passes / ctx.current_passes.size(),
                                                                   std::to_string(ctx.finished_passes) + "\\" + std::to_string(ctx.current_passes.size()) + " ("
                                                                       + std::to_string(ctx.new_unique_groupings.size()) + " new results)");
                             }
+                            m_progress_printer.print_progress_to_gui();
                         }
                     }
                 }
diff --git a/plugins/dataflow_analysis/src/utils/progress_printer.cpp b/plugins/dataflow_analysis/src/utils/progress_printer.cpp
@@ -13,7 +13,9 @@ namespace hal
 {
     namespace dataflow
     {
-        ProgressPrinter::ProgressPrinter(u32 max_message_size) : m_bar_width(0), m_max_message_size(max_message_size), m_terminal_width(get_terminal_width())
+        ProgressPrinter::ProgressPrinter(u32 max_message_size) : m_gui_message("dataflow analysis …"),
+              m_bar_width(0), m_max_message_size(max_message_size),
+              m_last_percentage(0), m_terminal_width(get_terminal_width())
         {
             if (m_terminal_width <= 8)
             {
@@ -28,15 +30,28 @@ namespace hal
             reset();
         }
 
-        void ProgressPrinter::print_progress(float progress, const std::string& message)
+        void ProgressPrinter::print_message_to_gui(const std::string& message)
         {
-            progress         = std::clamp(progress, 0.f, 1.f);
-            u32 int_progress = (u32)(progress * 100.0f);
-            if (GuiExtensionDataflow::s_progress_indicator_function)
+            m_gui_message = message;
+            print_progress_to_gui();
+        }
+
+        void ProgressPrinter::print_progress_to_gui(int percent)
+        {
+            if (!GuiExtensionDataflow::s_progress_indicator_function) return;
+            if (percent < 0) percent = m_last_percentage;
+            if (percent > 99) percent = 99;
             {
-                GuiExtensionDataflow::s_progress_indicator_function(int_progress < 100 ? int_progress : 99, "dataflow analysis running ...");
+                GuiExtensionDataflow::s_progress_indicator_function(percent, m_gui_message);
             }
 
+        }
+
+        void ProgressPrinter::print_progress_to_stderr(float progress, const std::string& message)
+        {
+            progress         = std::clamp(progress, 0.f, 1.f);
+            m_last_percentage = (u32)(progress * 100.0f);
+
             if (m_terminal_width <= 8)
             {
                 return;
@@ -82,7 +97,7 @@ namespace hal
             std::stringstream str;
             str << "[" << bar << "] ";
 
-            str << std::right << std::setw(3) << int_progress << '%';
+            str << std::right << std::setw(3) << m_last_percentage << '%';
 
             if (!print_message.empty())
             {
diff --git a/plugins/gui/include/gui/graph_widget/progress_bar.h b/plugins/gui/include/gui/graph_widget/progress_bar.h
@@ -31,6 +31,7 @@
 #include <QPushButton>
 #include <QImage>
 #include <QTimer>
+#include <QMutex>
 
 namespace hal {
 
@@ -64,12 +65,14 @@ namespace hal {
         Q_OBJECT
         QImage mImage;
         double mAngle;
+        QMutex* mMutex;
     public Q_SLOTS:
         void handleTimeout();
     protected:
         void paintEvent(QPaintEvent* event) override;
     public:
         BusyAnimation(QWidget* parent = nullptr);
+        ~BusyAnimation();
     };
 
     class BusyIndicator : public AbstractBusyIndicator
@@ -78,6 +81,7 @@ namespace hal {
         QTimer* mTimer;
         QLabel* mLabel;
         QProgressBar* mProgressBar;
+        QMutex* mMutex;
     public:
         BusyIndicator(QWidget* parent = nullptr);
         ~BusyIndicator();
diff --git a/plugins/gui/src/graph_widget/progress_bar.cpp b/plugins/gui/src/graph_widget/progress_bar.cpp
@@ -46,6 +46,7 @@ namespace hal {
     BusyIndicator::BusyIndicator(QWidget* parent)
         : AbstractBusyIndicator(parent)
     {
+        mMutex = new QMutex;
         QVBoxLayout* layout = new QVBoxLayout(this);
         layout->setAlignment(Qt::AlignHCenter);
         layout->addWidget(mLabel = new QLabel(this));
@@ -60,12 +61,18 @@ namespace hal {
     BusyIndicator::~BusyIndicator()
     {
         mTimer->stop();
+        delete mMutex;
     }
 
     BusyAnimation::BusyAnimation(QWidget* parent)
-        : QWidget(parent), mImage(QImage(":/icons/hal9000","PNG")), mAngle(0)
+        : QWidget(parent), mImage(QImage(":/icons/hal9000","PNG")), mAngle(0), mMutex(new QMutex)
     {;}
 
+    BusyAnimation::~BusyAnimation()
+    {
+        delete mMutex;
+    }
+
     void BusyAnimation::handleTimeout()
     {
         mAngle += 5;
@@ -76,50 +83,57 @@ namespace hal {
     void BusyAnimation::paintEvent(QPaintEvent* event)
     {
         Q_UNUSED(event);
-        QPainter p(this);
-        p.setRenderHint(QPainter::Antialiasing);
+        if (mMutex->try_lock())
+        {
+            QPainter p(this);
+            p.setRenderHint(QPainter::Antialiasing);
 
-        int rw = rect().width();
-        int rh = rect().height();
+            int rw = rect().width();
+            int rh = rect().height();
 
-        int w = rw > rh ? rh : rw;
-        int h = w;
+            int w = rw > rh ? rh : rw;
+            int h = w;
 
-        QRect rimg((rw-w)/2,(rh-h)/2,w,h);
+            QRect rimg((rw-w)/2,(rh-h)/2,w,h);
 
-//        p.fillRect(rimg,QBrush(Qt::gray));
-        QImage img = mImage.scaled(w,h);
-        double xc = w / 2.;
-        double yc = h / 2.;
+    //        p.fillRect(rimg,QBrush(Qt::gray));
+            QImage img = mImage.scaled(w,h);
+            double xc = w / 2.;
+            double yc = h / 2.;
 
-        for (int y = 0; y<h; y++)
-            for (int x=0; x<w; x++)
-            {
-                QRgb col = img.pixel(x,y);
-                if (col & 0xFF000000)
+            for (int y = 0; y<h; y++)
+                for (int x=0; x<w; x++)
                 {
-                    double angle = atan2(y-yc,x-xc) / M_PI * 180.;
-                    double da = angle >= mAngle ? angle - mAngle : angle + 360. - mAngle;
-                    int opaque = floor(da / 360.*256);
-                    opaque <<= 24;
-                    col = (col & 0xFFFFFF) | opaque;
-                    img.setPixel(x,y,col);
+                    QRgb col = img.pixel(x,y);
+                    if (col & 0xFF000000)
+                    {
+                        double angle = atan2(y-yc,x-xc) / M_PI * 180.;
+                        double da = angle >= mAngle ? angle - mAngle : angle + 360. - mAngle;
+                        int opaque = floor(da / 360.*256);
+                        opaque <<= 24;
+                        col = (col & 0xFFFFFF) | opaque;
+                        img.setPixel(x,y,col);
+                    }
                 }
-            }
 
-        p.drawImage(rimg,img);
+            p.drawImage(rimg,img);
+            mMutex->unlock();
+        }
     }
 
     void BusyIndicator::setValue(int percent)
     {
+        if (mProgressBar->value() == percent) return; // nothing to do
+        QMutexLocker lock(mMutex);
         mProgressBar->setValue(percent);
         update();
         qApp->processEvents();
     }
 
     void BusyIndicator::setText(const QString &txt)
     {
-        if (mLabel->text() == txt) return;
+        if (mLabel->text() == txt) return; // nothing to do
+        QMutexLocker lock(mMutex);
         mLabel->setText(txt);
         qApp->processEvents();
     }

Original file line number	Diff line number	Diff line change
`@@ -53,7 +53,7 @@ namespace hal`
`53`	`53`	`for (const auto& sequential_gate : netlist_abstr.target_gates)`
`54`	`54`	`{`
`55`	`55`	`cnt++;`
`56`		`- progress_bar.print_progress(cnt / netlist_abstr.target_gates.size());`
	`56`	`+ progress_bar.print_progress_to_stderr(cnt / netlist_abstr.target_gates.size());`
`57`	`57`
`58`	`58`	`auto current = sequential_gate->get_id();`
`59`	`59`
`@@ -140,7 +140,7 @@ namespace hal`
`140`	`140`	`for (u32 i = 0; i < stages_to_split; ++i)`
`141`	`141`	`{`
`142`	`142`	`++cnt;`
`143`		`- progress_bar.print_progress(cnt / stages_to_split);`
	`143`	`+ progress_bar.print_progress_to_stderr(cnt / stages_to_split);`
`144`	`144`
`145`	`145`	`std::unordered_map<u32, std::vector<u32>> move_out_reasons;`
`146`	`146`	`for (auto g : ctx.stages[i])`
`@@ -238,7 +238,7 @@ namespace hal`
`238`	`238`	`for (const auto& stage : directional_stages[i].stages)`
`239`	`239`	`{`
`240`	`240`	`cnt++;`
`241`		`- progress_bar.print_progress(cnt / (directional_stages[0].stages.size() + directional_stages[1].stages.size()));`
	`241`	`+ progress_bar.print_progress_to_stderr(cnt / (directional_stages[0].stages.size() + directional_stages[1].stages.size()));`
`242`	`242`	`for (auto it = directional_stages[1 - i].stages.begin(); it != directional_stages[1 - i].stages.end();)`
`243`	`243`	`{`
`244`	`244`	`auto& other_stage = *it;`
`@@ -296,7 +296,7 @@ namespace hal`
`296`	`296`	`std::set<std::vector<u32>> seen;`
`297`	`297`	`for (auto& [g, stages] : netlist_abstr.gate_to_register_stages)`
`298`	`298`	`{`
`299`		`- progress_bar.print_progress(++cnt / netlist_abstr.gate_to_register_stages.size());`
	`299`	`+ progress_bar.print_progress_to_stderr(++cnt / netlist_abstr.gate_to_register_stages.size());`
`300`	`300`
`301`	`301`	`if (stages.size() > 1)`
`302`	`302`	`{`
`@@ -336,7 +336,7 @@ namespace hal`
`336`	`336`	`cnt = 0;`
`337`	`337`	`for (const auto& combine : spread_stages)`
`338`	`338`	`{`
`339`		`- progress_bar.print_progress(++cnt / netlist_abstr.gate_to_register_stages.size());`
	`339`	`+ progress_bar.print_progress_to_stderr(++cnt / netlist_abstr.gate_to_register_stages.size());`
`340`	`340`
`341`	`341`	`for (auto stage_id : combine)`
`342`	`342`	`{`
Original file line number	Diff line number	Diff line change
`@@ -62,6 +62,7 @@ namespace hal`
`62`	`62`	`ctx.pass_counter = end_id;`
`63`	`63`	`if (ctx.pass_counter >= num_passes)`
`64`	`64`	`{`
	`65`	`+ std::lock_guard guard(ctx.result_mutex);`
`65`	`66`	`ctx.done = true;`
`66`	`67`	`}`
`67`	`68`	`}`
`@@ -72,13 +73,16 @@ namespace hal`
`72`	`73`
`73`	`74`	`if (auto it = ctx.pass_outcome.find({current_state, current_pass.id}); it != ctx.pass_outcome.end())`
`74`	`75`	`{`
`75`		`- // early exit, outcome is already known`
`76`		`- std::lock_guard guard(ctx.result_mutex);`
`77`		`- ctx.new_recurring_results.emplace_back(current_state, current_pass.id, it->second);`
`78`		`- ctx.finished_passes++;`
`79`		`- m_progress_printer.print_progress((float)ctx.finished_passes / ctx.current_passes.size(),`
	`76`	`+ {`
	`77`	`+ // early exit, outcome is already known`
	`78`	`+ std::lock_guard guard(ctx.result_mutex);`
	`79`	`+ ctx.new_recurring_results.emplace_back(current_state, current_pass.id, it->second);`
	`80`	`+ ctx.finished_passes++;`
	`81`	`+ m_progress_printer.print_progress_to_stderr((float)ctx.finished_passes / ctx.current_passes.size(),`
`80`	`82`	`std::to_string(ctx.finished_passes) + "\\" + std::to_string(ctx.current_passes.size()) + " ("`
`81`	`83`	`+ std::to_string(ctx.new_unique_groupings.size()) + " new results)");`
	`84`	`+ }`
	`85`	`+ m_progress_printer.print_progress_to_gui();`
`82`	`86`	`continue;`
`83`	`87`	`}`
`84`	`88`
`@@ -107,10 +111,11 @@ namespace hal`
`107`	`111`	`}`
`108`	`112`
`109`	`113`	`ctx.finished_passes++;`
`110`		`- m_progress_printer.print_progress((float)ctx.finished_passes / ctx.current_passes.size(),`
	`114`	`+ m_progress_printer.print_progress_to_stderr((float)ctx.finished_passes / ctx.current_passes.size(),`
`111`	`115`	`std::to_string(ctx.finished_passes) + "\\" + std::to_string(ctx.current_passes.size()) + " ("`
`112`	`116`	`+ std::to_string(ctx.new_unique_groupings.size()) + " new results)");`
`113`	`117`	`}`
	`118`	`+ m_progress_printer.print_progress_to_gui();`
`114`	`119`	`}`
`115`	`120`	`}`
`116`	`121`	`}`
Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,9 @@ namespace hal`
`13`	`13`	`{`
`14`	`14`	`namespace dataflow`
`15`	`15`	`{`
`16`		`- ProgressPrinter::ProgressPrinter(u32 max_message_size) : m_bar_width(0), m_max_message_size(max_message_size), m_terminal_width(get_terminal_width())`
	`16`	`+ ProgressPrinter::ProgressPrinter(u32 max_message_size) : m_gui_message("dataflow analysis …"),`
	`17`	`+ m_bar_width(0), m_max_message_size(max_message_size),`
	`18`	`+ m_last_percentage(0), m_terminal_width(get_terminal_width())`
`17`	`19`	`{`
`18`	`20`	`if (m_terminal_width <= 8)`
`19`	`21`	`{`
`@@ -28,15 +30,28 @@ namespace hal`
`28`	`30`	`reset();`
`29`	`31`	`}`
`30`	`32`
`31`		`- void ProgressPrinter::print_progress(float progress, const std::string& message)`
	`33`	`+ void ProgressPrinter::print_message_to_gui(const std::string& message)`
`32`	`34`	`{`
`33`		`- progress = std::clamp(progress, 0.f, 1.f);`
`34`		`- u32 int_progress = (u32)(progress * 100.0f);`
`35`		`- if (GuiExtensionDataflow::s_progress_indicator_function)`
	`35`	`+ m_gui_message = message;`
	`36`	`+ print_progress_to_gui();`
	`37`	`+ }`
	`38`	`+`
	`39`	`+ void ProgressPrinter::print_progress_to_gui(int percent)`
	`40`	`+ {`
	`41`	`+ if (!GuiExtensionDataflow::s_progress_indicator_function) return;`
	`42`	`+ if (percent < 0) percent = m_last_percentage;`
	`43`	`+ if (percent > 99) percent = 99;`
`36`	`44`	`{`
`37`		`- GuiExtensionDataflow::s_progress_indicator_function(int_progress < 100 ? int_progress : 99, "dataflow analysis running ...");`
	`45`	`+ GuiExtensionDataflow::s_progress_indicator_function(percent, m_gui_message);`
`38`	`46`	`}`
`39`	`47`
	`48`	`+ }`
	`49`	`+`
	`50`	`+ void ProgressPrinter::print_progress_to_stderr(float progress, const std::string& message)`
	`51`	`+ {`
	`52`	`+ progress = std::clamp(progress, 0.f, 1.f);`
	`53`	`+ m_last_percentage = (u32)(progress * 100.0f);`
	`54`	`+`
`40`	`55`	`if (m_terminal_width <= 8)`
`41`	`56`	`{`
`42`	`57`	`return;`
`@@ -82,7 +97,7 @@ namespace hal`
`82`	`97`	`std::stringstream str;`
`83`	`98`	`str << "[" << bar << "] ";`
`84`	`99`
`85`		`- str << std::right << std::setw(3) << int_progress << '%';`
	`100`	`+ str << std::right << std::setw(3) << m_last_percentage << '%';`
`86`	`101`
`87`	`102`	`if (!print_message.empty())`
`88`	`103`	`{`