Fix segmentation fault when selecting instance in debug mode

[krrish175-byte]

Description

Fixes a segmentation fault that occurs when trying to select an instance in GPL debug mode. This regression was introduced after a previous fix that added region-aware instance selection logic.

Fixes #9017

[gemini-code-assist]

Code Review

This pull request fixes a segmentation fault that occurs when selecting an instance in debug mode. The changes involve moving a loop to be guarded by a null check on the instance pointer and adding boundary checks before accessing a vector of NesterovBase objects. The fixes appear correct and address the reported issue. I've noted an opportunity to refactor duplicated code into a helper function to improve maintainability.

[gemini-code-assist]

This logic for determining nb_index is duplicated in reportSelected() at lines 530-539. To improve maintainability and avoid future inconsistencies, consider extracting this logic into a helper function. A free function within an anonymous namespace would be a good approach, following the repository's general rules.

For example:

namespace {
std::optional<size_t> getSelectedNesterovBaseIndex(
    size_t nb_selected_index,
    const std::vector<std::shared_ptr<gpl::NesterovBase>>& nbVec,
    utl::Logger* logger,
    int msg_id)
{
    size_t nb_index = 0;
    if (nb_selected_index != gpl::GraphicsImpl::kInvalidIndex) {
        nb_index = nb_selected_index;
    } else if (!nbVec.empty()) {
        logger->warn(
            utl::GPL, msg_id, "Selected instance not found in any NesterovBase");
    }

    if (nb_index >= nbVec.size()) {
        return std::nullopt;
    }
    return nb_index;
}
} // namespace

This helper could then be called from both drawNesterov and reportSelected.

References

1. Helper logic should be defined as a free function in a namespace rather than as a local lambda within a function.

[gudeh]

Hi @krrish175-byte, thanks for your contribution! Were you able to reproduce the issue? I don't remember what/when the issue is triggered exactly. I tried but couldn't reproduce the issue with master version on my end.

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[krrish175-byte]

Hi @gudeh! Yes, I was able to reproduce the issue on the master branch.

The segmentation fault occurs in GPL's debug mode if we try to select an instance that is not part of any NesterovBase (for example, an unplaced filler cell outside defined regions).

How to reproduce: If you create a simple test with regions and place a dummy filler cell outside of the region, selecting that filler with -inst in debug mode will trigger the crash immediately:

# Setup design with region, then:
set master [$lib findMaster "sky130_fd_sc_hd__fill_1"]
set dummy_inst [odb::dbInst_create $block $master "my_dummy_filler"]
$dummy_inst setPlacementStatus "PLACED"
$dummy_inst setOrigin 5000 5000

global_placement_debug -pause 1 -update 1 -inst my_dummy_filler
global_placement -density 0.7

In graphicsImpl.cpp, the debug initialization GraphicsImpl::debugForNesterovPlace() looks for the instance in nbVec_ to get nb_selected_index_. If it isn't found in any NesterovBase, nb_selected_index_ remains kInvalidIndex (which is std::numeric_limits<size_t>::max()).

Later, in GraphicsImpl::drawNesterov() and GraphicsImpl::reportSelected(), the code accesses nbVec_[nb_index] assuming it's valid, which leads to a segmentation fault:

    size_t nb_index = 0;
    if (nb_selected_index_ != kInvalidIndex) {
      nb_index = nb_selected_index_;
    } else {
      logger_->warn(
          utl::GPL, 317, "Selected instance not found in any NesterovBase");
    }
    FloatPoint densityGrad = nbVec_[nb_index]->getDensityGradient(gcell); // <--- SEGFAULT HERE!

When nb_selected_index_ is invalid, the code logs a warning but proceeds to use nb_index = 0 by default. And when it calls nbVec_[0]->getDensityGradient(gcell) on an unmapped gcell, it crashes.

The proposed fix (which I've pushed to fix/gpl-debug-select-inst) safely bails out of rendering if the index is invalid instead of defaulting to 0. Here is the screenshot:

[gudeh]

I believe the segmentation issue mentioned in #9017 was solved by commit ff6eed2. I wasn't able to reproduce the segmentation fault in any way, even with the dummy insertion mentioned by @krrish175-byte.

The code is only missing some polishing after being solved with the mentioned commit:

We find the selected instance in two different places in two different ways, would be nice to use the same methodology, maybe a private member function.

We can also make sure we don't use the nb_index when it's invalid.

[gudeh]

Why include this namespace? getSelectedNesterovBaseIndex() should be a private member function of GraphicsImpl. No need for all these parameters.

[gudeh]

I understand the code is only missing a proper protection from an invalid nb_index (I don't see it happening on my tests, but would be nice to have). I mean, when nb_selected_index_ == kInvalidIndex. Maybe we don't even need the nb_index variable.

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[krrish175-byte]

@gudeh, PTAL