Fixups for CUS-360

[akashlevy]

No description provided.

[linear]

CUS-360

[greptile-apps]

Greptile Summary

This PR delivers targeted fixes for CUS-360, addressing memory-safety bugs in the liberty-defined generated clock path and hardening two waveform-generation helpers in Clock.cc.

Key changes:

• Double-free / use-after-free fix (Sdc.cc): createLibertyGeneratedClocks now deep-copies edges and edge_shifts before handing them to makeGeneratedClock, which takes ownership. Previously, when a clock was redefined, the old Clock object was destroyed (freeing edges_), while the GeneratedClock still held a pointer to that same memory — causing a double-free or use-after-free on the next redefinition.
• generateEdgesClk refactor (Clock.cc): The nested if/else is replaced with an early-return guard (size < 3). The edge_shifts_ application is now gated on a size >= 3 check (has_shifts), preventing out-of-bounds access when edge_shifts_ contains fewer than 3 entries.
• masterClkEdgeTr null/size guard (Clock.cc): A missing null and bounds check is added before indexing into edges_, preventing a potential null-pointer dereference when edges_ is absent or shorter than expected.
• New regression test (generated_clock_edges_redefine.*): Exercises the fixed scenario by redefining the same master clock three times and verifying that the liberty-inferred generated clocks track the new period correctly each time.

Issues found:

• When edge_shifts_ is non-null but has fewer than 3 entries, all shifts are silently discarded with no diagnostic message. A warning would help diagnose misconfigured inputs.
• A latent (pre-existing) modulo-by-zero risk exists if src_wave is empty; this is a good opportunity to add a guard given the surrounding refactor.

Confidence Score: 4/5

• Safe to merge — fixes real memory-safety bugs with correct logic and adequate test coverage; two minor style-level gaps remain.
• The double-free and use-after-free fix in Sdc.cc is correct and well-tested by the new regression. The Clock.cc guards are logically sound. The two flagged items (silent shift suppression and a pre-existing modulo-by-zero) are style/robustness suggestions, not blockers.
• sdc/Clock.cc — the has_shifts guard silently discards partial edge shifts; consider adding a warning. The modulo-by-zero risk on an empty source waveform is also worth addressing here.

Important Files Changed

Filename	Overview
sdc/Clock.cc	Refactors generateEdgesClk to use an early-return guard and adds a has_shifts size check to avoid out-of-bounds access on edge_shifts_; adds a null/size guard in masterClkEdgeTr to prevent null dereference. Minor concerns: partial edge_shifts_ is silently ignored without a warning, and a latent modulo-by-zero on empty src_wave is not addressed.
sdc/Sdc.cc	Introduces deep copies of edges and edge_shifts before passing them to makeGeneratedClock, preventing double-free / use-after-free when a liberty-defined generated clock is redefined. Fix is correct and well-commented.
test/generated_clock_edges_redefine.tcl	New regression test that exercises createLibertyGeneratedClocks through three successive clock redefinitions to stress-test ownership of edges/edge_shifts pointers; adequately covers the double-free scenario fixed in Sdc.cc.
test/generated_clock_edges_redefine.lib	New Liberty library defining a cell with a liberty-level generated clock using edges(1,3,5); correctly structured to exercise the edge-based waveform generation path.
test/generated_clock_edges_redefine.ok	Expected output file; waveform periods (20/40/80 and 10/20/40 and 5/10/20 ns) are consistent with the edge-based period calculation from a source clock of 10/20/5 ns.
test/generated_clock_edges_redefine.v	New Verilog stub instantiating two CLK_EDGE_GEN cells in a chain (CLK_IN → CLK_MID → CLK_OUT) to exercise nested liberty-defined generated clocks.
test/regression_vars.tcl	Adds generated_clock_edges_redefine to the public regression test list; straightforward, no issues.

Sequence Diagram

sequenceDiagram
    participant TCL as Tcl (create_clock)
    participant SDC as Sdc::makeClock
    participant LGC as Sdc::createLibertyGeneratedClocks
    participant GC  as GeneratedClock (Liberty)
    participant MK  as Sdc::makeGeneratedClock
    participant CLK as Clock (runtime object)

    TCL->>SDC: create_clock -period P [pin]
    SDC->>SDC: deletePinClocks / deleteClkPinMappings (frees old Clock → edges_ freed)
    SDC->>LGC: createLibertyGeneratedClocks(clk)
    LGC->>GC: edges(), edgeShifts()
    note over LGC: Deep-copy edges & edge_shifts<br/>(fix: prevents use-after-free on next redefinition)
    LGC->>MK: makeGeneratedClock(..., edgesCopy, edgeShiftsCopy)
    MK->>CLK: initGeneratedClk(edges=edgesCopy, ...)
    CLK->>CLK: generateEdgesClk(src_clk)<br/>has_shifts = edge_shifts_ && size>=3<br/>masterClkEdgeTr: null/size guard

Loading

_{Last reviewed commit: "Fixups for CUS-360"}

[greptile-apps]

Silent suppression of partial edge_shifts_ without warning

has_shifts is set to false whenever edge_shifts_ has fewer than 3 entries but is still non-null. This means a non-empty edge_shifts_ vector with 1 or 2 entries will be silently ignored — no warning is emitted and the caller gets no feedback that their edge-shift data was discarded.

The old code suffered the opposite extreme (undefined behaviour by accessing out-of-bounds indices), but the new approach should at least warn when this edge case is hit, to make debugging easier:

Suggested change

	bool has_shifts = edge_shifts_ && edge_shifts_->size() >= 3;
	bool has_shifts = edge_shifts_ && edge_shifts_->size() >= 3;
	if (edge_shifts_ && !has_shifts)
	Sta::sta()->report()->warn(245, "generated clock edge_shifts size is less than three; shifts ignored.");

Rule Used: Always add comments to explain the purpose of comp... (source)

Learnt From
Silimate/preqorsor-vscode#33

[greptile-apps]

Potential modulo-by-zero if source waveform is empty

src_size is of type size_t and is derived from src_wave->size(). If the source clock has an empty waveform (size == 0), the expression edge0_1 % src_size is a modulo-by-zero operation (undefined behaviour in C++). The same pattern is repeated for edge1_1 and edge2_1.

This is a pre-existing condition that was not introduced by this PR, but since the function is being refactored here it is a good opportunity to add a guard:

  if (src_size == 0) {
    Sta::sta()->report()->warn(245, "generated clock source waveform is empty.");
    return;
  }