Fix crash on Verilog escaped-identifier ports containing /#10275
Conversation
…y delimiter, fix crash Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies dbModule::findModBTerm to correctly handle port names containing hierarchy delimiters, such as escaped identifiers, by attempting a full-name lookup before falling back to the trailing segment. A new regression test, hier_weird_port, has been added to verify the fix. Feedback recommends optimizing the lookup logic to minimize std::string allocations by using strrchr to identify the hierarchy delimiter directly on the input character pointer.
| auto it = obj->modbterm_hash_.find(std::string(name)); | ||
| if (it == obj->modbterm_hash_.end()) { | ||
| std::string leaf(name); | ||
| const char hier_delimiter = getOwner()->getHierarchyDelimiter(); | ||
| size_t last_idx = leaf.find_last_of(hier_delimiter); | ||
| if (last_idx != std::string::npos) { | ||
| leaf = leaf.substr(last_idx + 1); | ||
| it = obj->modbterm_hash_.find(leaf); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation performs multiple std::string allocations and copies (one for the initial lookup, one for the leaf copy, and one for the substr). This can be optimized by using strrchr to find the hierarchy delimiter directly on the input const char* and only performing a second lookup if necessary. This avoids redundant string constructions and improves both performance and readability.
auto it = obj->modbterm_hash_.find(name);
if (it == obj->modbterm_hash_.end()) {
const char hier_delimiter = getOwner()->getHierarchyDelimiter();
const char* last_delim = strrchr(name, hier_delimiter);
if (last_delim != nullptr) {
it = obj->modbterm_hash_.find(last_delim + 1);
}
}
There was a problem hiding this comment.
Good catch on the leaf-path string ops — the strrchr + last_delim + 1 form is cleaner and skips one allocation when the fallback fires. Note though that the first lookup isn't a savings: find(name) on
unordered_map<string, …> still implicitly constructs a std::string for the key compare, identical to the prior explicit std::string(name) — applied for the readability, not as a hot-path perf fix.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
All clean here |
|
|
||
| // Try the full name first. modbterm names may legitimately contain the | ||
| // hierarchy delimiter (e.g. escaped-identifier ports emitted by Fusion | ||
| // Compiler), so unconditionally truncating at the last '/' would miss |
There was a problem hiding this comment.
How about removing the specific commercial tool name in the comment?
Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
maliberty
merged commit 512e44c
into
The-OpenROAD-Project:master
4 participants
Summary
Problem
check_setup -verbose (and any path through dbNetwork::direction → dbModBTerm::getIoType) crashes with SIGSEGV on netlists where a module port name is a SystemVerilog escaped identifier containing /.
Concretely, Fusion Compiler emits port declarations like:
This is a legal SV escaped identifier and OpenSTA's Verilog reader correctly preserves it. The resulting dbModBTerm is stored in dbModule::modbterm_hash_ keyed by the literal name (with / characters intact).
However, dbModule::findModBTerm unconditionally truncated the lookup key at the last / before hashing, so the lookup missed → caller dereferenced a null dbModBTerm → crash.
Backtrace:
Fix
Single change in src/odb/src/db/dbModule.cpp:545 (dbModule::findModBTerm): try a full-name hash lookup first, and fall back to the trailing-segment lookup only when the full name misses. This handles modbterms whose stored names legitimately contain the hierarchy delimiter (escaped-identifier ports), without altering behavior for any caller that was already passing a hierarchical path to a non-hierarchical port name.
Files changed
Type of Change
Impact
Fixes a crash in a SiliconCompiler testcase and is reproducible in the small TCL added in this PR
Verification
./etc/Build.sh).Related Issues