Add watermarking module (src/wmk) - #11160
Open
ytliu8464 wants to merge 35 commits into
Open
Conversation
Add the wmk module implementing the routing watermark of Kahng et al., "Robust IP Watermarking Methodologies for Physical Design" (ISPD'98). A keyed HMAC-SHA256 selects a subset of signal nets, tags them with a "watermark" dbBoolProperty, and the detailed router inflates the non-preferred-direction grid cost on those nets so they use measurably less wrong-way wiring than the rest of the design. Commands: set_routing_watermark, report_routing_watermark, clear_routing_watermark, and set/get_routing_watermark_strength. The grid cost scaling applies only to the gridCost term; apCost keeps its stock value, since inflating an access-point penalty would degrade pin access on exactly the nets carrying the watermark. WATERMARK_WRONGWAY_MULT is serialized with the rest of RouterConfiguration so distributed workers see the configured value. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
Check committed placement and CTS claims against the loaded design, so ownership can be established from a bare OpenROAD session without the Python verifiers and their dependencies. The key is not needed: it was consumed at embed time to derive the target values, which the claim file records. Verification re-observes each claimed object -- which cell of a pair sits further left, and the parity of a leaf clock buffer's sequential fanout -- and reports the extraction rate. Ownership is decided against a threshold rather than by exact match, because routing and filling disturb a few marked objects. Only sequential sinks are counted for the CTS parity. The embedder also classifies repair and other sinks, but that distinction decides which buffers are eligible to be marked, not what a marked one now reads. Claim columns are looked up by name so a schema that gains a column does not silently shift values. Verified against the Python verifiers on jpeg/nangate45: both report placement 101/102 (r_P=0.9902) and CTS 32/32 (r_C=1.0000), agreeing on which claim was disturbed. On the un-watermarked baseline the same claims give r_P=0.5196 and r_C=0.6562, at chance for a binary carrier. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
Formatting only, no functional change. The module was written against an older configuration than this tree's. Per AGENTS.md the SWIG interface is left alone. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
Document the commands in the format docs/contrib/ReadmeFormat.md requires,
and add three integration tests registered in both CMake and Bazel:
select keyed selection is deterministic, key-sensitive, and clearing
removes exactly what was tagged
strength the wrong-way multiplier round-trips through router config
verify extraction rate decides the verdict -- a claim that no longer
holds does not by itself sink it, a claim the embedder skipped
is not counted, and the same evidence fails once the threshold
is raised past it
The verify fixture is generated from the test design rather than invented,
so its target bits describe real cell positions.
Signed-off-by: Yiting Liu <yil375@ucsd.edu>
Add the Python interface so the watermark commands are available in "openroad -python" as well as Tcl. This matters for this module in particular: the placement and CTS embedders run under the Python interpreter, so they can now check their own work in the same process rather than shelling out. Reached through Design like every other tool, so design.getWatermark().verifyPlacement(claims) reads the same way as design.getExample(). odb is linked explicitly because wmk links it privately and the public header includes odb/db.h. Verified against the Tcl commands on jpeg/nangate45: both surfaces report placement 101/102 and CTS 32/32. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
test/results holds ctest scratch and should not be tracked; matches the .gitignore in src/exa. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
Drop the unkeyed selection path. set_routing_watermark took either -key_hex or a public -message string, the latter selecting nets with a mt19937 seeded from that string. A watermark whose marked set can be predicted without the key defeats the point of keying the selection, and the option had no test. -key_hex is now required. Add bias.tcl and bias_neutral.tcl, which route one design twice -- identically except for the wrong-way cost multiplier -- and record the resulting wrong-way wirelength: 460 DBU with the multiplier neutral, 0 with it raised. Nothing else in the suite fails if the grid cost hook stops reaching the maze router, because selection, reporting and verification would all still be correct while the watermark itself became inert. Confirmed by disabling the hook: bias.tcl fails, and bias_neutral.tcl correctly still passes because at strength 1 the multiplier is already a no-op. Every net is tagged in those tests so they measure the multiplier alone, independent of which nets a given key selects; select.tcl covers that. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
Add TestHmacSha256, checking the vendored HMAC-SHA256 against the RFC 4231 vectors, including the case where the key is longer than the block size and must be hashed first -- the one a naive implementation gets wrong. The watermark selects nets with this PRF and a verifier only agrees with an embedder if both compute the same digest, so it is worth pinning to published vectors rather than to our own output. Also covers parse_hex_key32, which rejects anything that is not 64 hex characters. Correct the comments around the wrong-way multiplier. They claimed it scales non-preferred-direction edges, but it scales whatever getCosts charges GRIDCOST on, and FlexGridGraph::setGridCostE is called both for non-preferred-direction edges and for preferred-direction edges that are off-track or on a worker border. Wrong-way edges are a subset, so the watermark still shows up in the wrong-way statistic the detector measures, but the penalty is broader than that statistic alone and the comments should not imply otherwise. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
gtest_discover_tests writes googletest_discovery_*.json next to the test source, which should not be tracked. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
verify_watermark reads claim files, but the format was described only as "claim file from the placement watermark". An embedder is free to be any tool, so the format it must produce belongs here, in the module that consumes it, rather than being defined implicitly by whichever producer happened to come first. Documents the columns actually read, that they are matched by name so order is free and extra columns are ignored, how skipped_reason and final_bit exclude a row, and that a claim naming an absent instance counts against the extraction rate rather than aborting the check. Verified by rebuilding the claim files with only the documented columns in a scrambled order: same verdict, 5/5 and 2/2. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
The module could select and bias the routing watermark but relied on
external tooling for everything else. It now embeds the placement and
clock tree marks as well, and checks all three, so a design can be
watermarked and its ownership proved without leaving OpenROAD.
place_watermark puts a keyed subset of same-row same-width cell pairs
into a keyed left-to-right order. cts_watermark drives a keyed subset of
leaf clock buffers to a keyed sequential-fanout parity by moving one sink
across the boundary between two buffers. verify_watermark gains the
routing stage, whose marked set it recovers from the key alone.
Three things decide whether the extraction rate these report is evidence
of anything, and all three are easy to get wrong in the direction that
looks good:
- Which objects a stage marks is settled before the key is consulted.
An embedder free to pair with whichever cell already sat in the keyed
order would score a perfect rate on a design it had never touched.
- Every object chosen is claimed, including the ones whose mark did not
take. Dropping them lets the embedder pick its evidence after seeing
the design, with the same result.
- The verifier does not consult a claim's own record of how its
embedding turned out. Honouring it would let a claim file choose its
own denominator, and a rate computed that way is 1 everywhere.
Together these put the rate at one half on a design the key did not mark,
which is what makes a rate above the threshold mean something. Measured
on jpeg: 1.0000 on the marked design, 0.4766 on the same design before
embedding, where the claims that hold are exactly the pairs that had
already been in the keyed order by chance.
Routing needs a different rule because it has no claims to count. It was
judged on the sign of T_R, which is a coin flip on an unwatermarked
design -- the baseline jpeg run reports T_R = -0.004807 and would have
been accepted. The stage now reports a randomization p-value against the
null of drawing the marked set at random, and is judged against alpha,
which rejects that baseline at p = 0.145. Sampling floors p at 1/(B+1),
far above what a working watermark reaches, so the closed-form tail is
reported alongside and the smaller of the two decides: the watermarked
jpeg gives 1e-515.9 where sampling bottoms out at 1e-5.
Signed-off-by: Yiting Liu <yil375@ucsd.edu>
Signed-off-by: Yiting Liu <yil375@ucsd.edu>
Every test here is paired with one that must fail, because a watermark
test that only checks its own design is the easiest kind to write and the
least useful: verification against the design the embedder just modified
passes whether or not anything was embedded.
place / place_unmarked embed, then check the same claims against the
design as it was beforehand
cts / cts_guard embed, then embed under a skew budget one pair
cannot meet
route_verify / route_stat route under the bias, then measure a routed
design that carries no watermark
Each was confirmed to fail when the property it guards is removed:
judging routing on the sign of T_R breaks route_stat; letting final_bit
exclude a claim breaks cts_guard; claiming only the pairs that already
matched breaks place; dropping the claims that do not hold breaks
place_unmarked and verify; disconnecting the cost multiplier from the
maze router breaks bias and route_verify.
gcd_placed.def is gcd.def after detailed_placement. The placement mark
is an ordering within a row, so it needs a legal placement to mean
anything, and checking one in keeps the test measuring the watermark
rather than the placer.
The four tests that run a placer, a clock tree synthesizer or a router
check their own numbers instead of a golden log, so that those tools'
output does not end up pinned inside a watermarking test.
Signed-off-by: Yiting Liu <yil375@ucsd.edu>
Signed-off-by: Yiting Liu <yil375@ucsd.edu>
The randomization draws ran over the marked and unmarked q_R values in the order they were collected, so the realized sample depended on which nets the key had picked. The null is over relabellings of the same multiset and should not move with the key: sorting it means two keys that select the same number of nets face the same null. Signed-off-by: Yiting Liu <yil375@ucsd.edu> Signed-off-by: Yiting Liu <yil375@ucsd.edu>
The candidate loop read a cell's position before checking whether it had already been paired. A committed swap moves both of its cells, so the sort order only still describes the cells left over, and a moved one appearing in the window would stop the search early. Also renames the counter that reported cells finding no partner at all: it was labelled a distance rejection, which is only sometimes the reason. Signed-off-by: Yiting Liu <yil375@ucsd.edu> Signed-off-by: Yiting Liu <yil375@ucsd.edu>
sha256 and hmac_digest went in with the placement, clock tree and routing work and neither was covered. Both decide what gets marked, so a change that silently altered either would move every keyed selection and only show up as watermarks that no longer verify. sha256 is checked against the two published vectors. hmac_digest is checked for the property it exists to provide: the parts are length-prefixed, so re-splitting them changes the digest. Without that the parts would just be concatenated and one cell pair could be made to collide with another and take its keyed bit. Embedded NULs are covered too, since the placement watermark feeds tile coordinates in as raw bytes and the first tile's are almost all zero. Signed-off-by: Yiting Liu <yil375@ucsd.edu> Signed-off-by: Yiting Liu <yil375@ucsd.edu>
Follows docs/contrib/ReadmeFormat.md and the shape the other module READMEs use: mandatory flags sorted alphabetically alongside the optional ones, the standard regression-tests wording, and a References section. Drops the FAQ heading, which only two of thirty-four modules carry and which said nothing the issue tracker does not. Documents place_watermark and cts_watermark, which had no entry, and states plainly that keys are the caller's to produce and keep: the module reads one, uses it and forgets it. Signed-off-by: Yiting Liu <yil375@ucsd.edu> Signed-off-by: Yiting Liu <yil375@ucsd.edu>
…test data regression_test defaults check_log to true, so the four tests that assert their own numbers instead of carrying a golden would have been diffed against .ok files that do not exist. They pass under CMake and fail under Bazel -- the exact split docs/agents/testing.md warns about. Marks them the way gpl and ram do. Also drops two symlinks that reached into other modules' test directories, which nothing upstream does. drt/test/sky130hd is itself only a symlink to test/sky130hd, so this points there directly, and the routed design comes from test/gcd_nangate45.def rather than rcx's copy. That is a different design, so route_stat's two keys were rechosen: the one whose T_R happens to come out negative here is the 5555 key, and the ordinary case is 3333. Signed-off-by: Yiting Liu <yil375@ucsd.edu> Signed-off-by: Yiting Liu <yil375@ucsd.edu>
Both embedders picked their objects by position -- placement paired cells adjacent along the row, CTS paired the nearest free buffers -- and used the key only to decide the value each object was driven to. The marked set was therefore computable by anyone holding the design, which is the wrong half of the scheme to leave public: an observer who knows the algorithm could list every marked pair and would only have to guess its bit. Candidates are now enumerated and screened by the public rules -- same row, same width, close enough, no timing or wirelength cost for placement; same clock and within range for CTS -- and the key then orders what survives, with a greedy non-overlapping prefix taken from that order. So the eligible set stays public and the marked subset does not. The ordering is keyed on the objects' names, never on the values they currently show, which is what keeps the extraction rate meaningful. On jpeg, two keys differing in one bit now share only 94 of 256 marked pairs; before, both picked the same 256. CTS also follows the paper more closely: a sink is taken from the source buffer and given to the target rather than the other way round, and only the claimed target is frozen afterwards, so a buffer that merely lent a sink can still be paired again. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
Without a way to produce a key there is no way to use any of this: the
commands all take one and nothing made one. generate_watermark_key
draws a secret and a nonce from the system's random source and derives
the three stage keys,
K_s = HMAC-SHA256(K, design_id, nonce, "stage=" || s)
and derive_watermark_key recovers one of them later, so verification in
a separate process needs only the secret plus two public values and no
stored stage keys. The draw fails rather than falling back when
/dev/urandom cannot be read -- std::random_device is not required to be
non-deterministic, so it must not be used for this -- and the secret is
never logged.
verify_watermark now grants ownership on at least -min_stages of the
stages checked, two by default, which is the scheme's rule. Requiring
every stage let one stage with no capacity sink a claim the other two
proved: on gcd a key whose three clock-tree moves all cost skew failed
the whole verdict while placement stood at 1.000. Checking a single
stage still works with -min_stages 1 and is what the tests do.
Certificate sealing and the timestamped key commitment are deliberately
not here. Their guarantee comes from an independent timestamping
authority rather than from anything the tool can check, they need
network access and key custody, and OpenROAD carries no crypto
dependency today. The claim files and the stage keys are the interface
to whatever provides them.
Signed-off-by: Yiting Liu <yil375@ucsd.edu>
ASAP7 routes strictly in the preferred direction: on a routed aes, all 12613 signal nets carry zero wrong-way metal, where the same design on NanGate45 has 8943 of 15649 using some. The statistic was computing T_R = 0 over that and reporting 'routing shows no watermark', which reads as a missing mark when the truth is that the technology offers nothing to mark. The stage now detects that no eligible net uses wrong-way metal at all, says so, and is skipped rather than counted as a failed stage, so ownership falls to placement and CTS -- which is how the scheme treats that platform anyway. Exercised on ASAP7 but not covered by a regression test: none of the checked-in routed designs is free of wrong-way metal, and building one by hand to reach a single branch did not seem worth a fixture. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
Reading a design from a database restores no liberty and no constraints, so worstClockSkew returns zero, and zero is indistinguishable from a clock with no skew. The guard compared zero against zero, never rejected anything, and committed every mark without having checked the clock at all -- silently. The difference is not small. On a routed ASAP7 aes the guard turns away 14 of 26 pairs when timing is set up; the same design read bare accepts all 26. Anyone diagnosing capacity from the second number would draw the wrong conclusion, which is what happened here. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
The margin defaulted to zero, demanding that moving a sink cost no skew at all. A sink move almost always costs a little, so on a tight clock the guard turned away nearly everything: on a routed ASAP7 aes -- 380 ps period, 7.8 ps skew -- 14 of 26 pairs were rejected, the extraction rate came out at 0.46, and the owner could not prove ownership of their own design. With routing structurally unavailable on that platform, the whole claim failed. The reference implementation allows 20 ps (WM_CTS_SKEW_SLACK_PS), which is where this now sits. The same design then marks all 26 pairs with nothing rejected, for about 1 ps of skew. cts_guard asks for zero explicitly, since that is what makes the guard observable on a four-buffer tree. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
…ks that cost timing Three things the reference implementation does and this did not. The clock tree mark now asks the library whether a buffer can take another sink, instead of only asking whether the clock skew moved. Slew and capacitance limits come from the liberty cell through OpenSTA, so the answer follows the technology; 20% of each must remain unused. At that default nothing is rejected on gcd, on a routed NanGate45 aes, or on ASAP7, which is the healthy reading -- the marks are nowhere near what the buffers can take. cts_drive asks for 99% instead, where all three gcd pairs are turned away, because a check that never fires is indistinguishable from one that was never wired up. Placement retries when the first pass finds fewer than -min_pairs_total pairs, widening the neighbourhood and doubling the wirelength budget. Both passes run against the untouched design and the second only adds, so the choice is still the key's. On gcd it turns 20 pairs into 24. On the aes designs it adds nothing, and the message says so: there the binding constraint is the slack screen, which rejects 9801 of 14820 cells on NanGate45, and no amount of widening the neighbourhood reaches a cell that was never a candidate. And a pair whose cells lose more than 20 ps of slack once the design is legal again is put back and not claimed. What is undone depends on timing and not on the bits the pairs carry, so the claims that remain are still chosen without reference to what the design shows. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
The check compared each cell's slack before and after its swap, but moving a cell changes only its parasitics and nothing recomputes those until something asks. Both readings therefore came from the same cache and were identical, so nothing was ever put back: at a tolerance of one femtosecond, all 48 cells across 24 pairs compared exactly equal, which is not a result a real measurement produces. The module now re-estimates parasitics from the placement before re-reading slack, the way any other consumer of post-placement timing does. The same run then puts 18 of the 24 pairs back at that tolerance, and none at the 20 ps default, which is the honest answer for a design whose swaps cost well under that. place_guard covers it at 1 ps, where two pairs are undone. Testing at the default would have proved nothing -- a guard that never fires and a guard that cannot see look identical from outside, which is exactly how this one escaped notice when it was written. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
Every keyed method takes a 32-byte key as a std::array, and nothing in Python builds one, so the module imported and the class appeared but each call raised TypeError. Verification happened to work -- it takes a file name -- which made the surface look half-present rather than broken, and no test noticed because they all drive the Tcl commands. A key is now accepted as the same 64-character hex string the Tcl commands take, or as 32 raw bytes. A string of the wrong length, bytes of the wrong length, or something that is not either raises instead of being padded or truncated into a key, which would otherwise produce a watermark nobody could reproduce. python_api covers it and was checked against the typemap being removed again, where it is the only test that fails. Key generation is still reached through evalTclString; the README says so. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
The module was CMake-only. It had no BUILD file and appeared nowhere in
the root one, so a Bazel build of OpenROAD failed outright on Design.cc,
which includes wmk/Watermark.h for getWatermark. CMake never noticed
because the include path arrives transitively through the link.
Running Bazel for the first time found four separate faults, none of
which CMake can express:
- the module was absent from the build entirely
- layering_check wants the target that exports a header, not one that
merely links it: db_sta/dbNetwork.hh comes from //src/dbSta:dbNetwork
and not from //src/dbSta
- PyUnicode_AsUTF8 is outside the limited Python API that Bazel
compiles the extension against, so the key typemap did not compile
there at all; it now goes through a bytes object, which is inside it
- the Python module needs an extension and a py_library packaged
together, and its SWIG module has to be named for what it is imported
as, which differs between the two builds
All nine tests that check their own numbers now pass under both. The
seven that compare against a golden log fail here for an environmental
reason and not a real one: this host cannot reach its NIS server from
inside the sandbox, and the resulting line lands in every captured log.
Upstream //src/exa/test:basic-tcl_test fails identically, with the same
one-line diff.
python_api turns off the slack screen and the post-embed guard, because
what it tests is that a key crosses into C++ and an answer comes back.
How many pairs a design yields under timing pressure is place.tcl's
business, and leaving those on made the counts depend on how timing
happened to be set up, which is not the same in both builds.
Signed-off-by: Yiting Liu <yil375@ucsd.edu>
The previous one ran to 517 lines against dpl's 238 and cts's 274, and argued its case in prose where the other modules state theirs. Command descriptions are now one to three sentences, option entries are one line, and the reasoning that belonged in the source has gone back to it. Follows grt and gpl for the sections a module carries and their order, including 'Using the Python interface to wmk' after Limitations. Cites the paper the module implements, which the old text did not: Kahng and Liu, Kerckhoffs-Compliant Watermarking for Physical Design IP Protection: From Placement to Routing, arXiv 2608.05055. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
src/wmk/README.md existed but nothing referenced it, so it would not have appeared on the documentation site. docs/toc.yml is what puts a module there, and every comparable tool -- ant, cgt, fin, ram -- is listed in it. The two modules absent from that file are exa, which is a template, and web. This is neither. Not render-tested: sphinx is not installed here, so this is checked only by the file parsing and its target existing. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
The README used the word eight times and never defined it, which is no help to a reader who does not already know the scheme. It identifies one watermark instance: same secret, same design, different nonce marks different objects with different values. Checked on gcd, where two nonces over one secret share 2 of 5 marked pairs. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
Signed-off-by: Yiting Liu <yil375@ucsd.edu>
The verifier read each claimed bit with atoi, which answers zero for a field that is empty or not a number. A claim file with an empty target_bit column therefore scored against zero and, when the design happened to show zero, was reported as holding: [INFO WMK-0034] Placement watermark: 1 / 1 claims hold (r_P=1.0000). [INFO WMK-0045] Ownership evidence holds in 1 stage(s). That is a verdict of ownership drawn from a file that committed to nothing. A claimed bit is now required to be exactly "0" or "1", and a file that says anything else is refused rather than scored. The same file also carried its own copies of singleOutputNet, isSequentialClockSink and seqFanout. ClockTree.h exists so that the embedder and the verifier cannot disagree about what a leaf clock buffer's sequential fanout is -- the parity of that count is the mark -- and a second definition sitting beside it is one edit away from the drift the header warns about. The copies are gone; both sides call the shared ones. verify.tcl feeds the verifier a row with an empty target_bit and expects the refusal. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
The module reported 88 findings against upstream's zero. None changed
what the code does; leaving them would have made the module the noisiest
directory in the tree.
std::sort / transform / max_element -> std::ranges:: (10)
static_cast to silence a sign comparison -> std::cmp_* (3)
C-style casts, int multiplication used
as a pointer offset -> static_cast, size_t (7)
count() != 0, nested ternary, positional
initializers, an unused using-decl (5)
headers used but not included, or
included and not used (55)
wmk was also missing from HeaderFilterRegex, so nothing in the module's
headers was ever analysed. Added.
The count was 89 in an earlier reading and is not comparable: six of the
ten sources failed to parse in that run, so several checks never ran at
all -- including the one that found the atoi in the verifier.
Signed-off-by: Yiting Liu <yil375@ucsd.edu>
The README says secret key throughout; the sources still said secret. Aligned in watermark.tcl, Keys.h, Keys.cpp, watermark.i and keygen.tcl. Left alone where the word is an adjective and reads correctly as one -- "depends on nothing secret", "a key drawn from a predictable source is not secret". Also carries the arXiv number into the README's reference entry. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
The design identifier and the nonce are public, and both are needed again to derive the stage keys. Until now the only record of them was the file -file writes, which also holds the secret key at owner-only permissions. An owner who wanted to hand a verifier the public inputs had to pick them out by hand, and the obvious shortcut was to pass on the whole file. -public_file writes just those two, at ordinary permissions, so the secret file never has to leave the owner. keygen.tcl checks that the file names both values and that no key material reaches it -- a stage key appearing there would make it exactly as dangerous to pass on as the file it exists to replace. route_verify.tcl now drops the net tags and verifies again. The routing stage recovers its marked set from the key and never reads the tags back out of the design, but nothing tested that, and a flow that clears the tags before shipping depends on it. Also carries the README wording changes made alongside these. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
tclfmt wanted a two-space continuation indent in six files, and two comment lines ran past the hundred-column limit. The Tcl lint job pins tclint 0.7.0 and fails the build on either, so this would have bounced before anyone read the PR. python_api.csv and python_api_bytes.csv were committed by accident. The Python test writes its claims through helpers.make_result_file, which puts them in results/ under a different name, so nothing ever read these two. Signed-off-by: Yiting Liu <yil375@ucsd.edu>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Welcome to OpenROAD! Thanks for opening your first PR.
Before we review:
- Contribution Guide: https://openroad.readthedocs.io/en/latest/contrib/contributing.html
- Build Instructions: https://openroad.readthedocs.io/en/latest/contrib/BuildWithCMake.html
Please ensure:
- CI passes
- Code is properly formatted
- Tests are included where applicable
A maintainer will review shortly!
Contributor
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
Member
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 693fa4f1c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
Comment on lines
+156
to
+158
| for (size_t i = 0; i < lcbs.size(); ++i) { | ||
| for (size_t j = i + 1; j < lcbs.size(); ++j) { | ||
| if (manhattan(lcbs[i], lcbs[j]) > max_dist) { |
There was a problem hiding this comment.
In designs with multiple clocks, candidate pairs are filtered only by physical distance, even though findLeafClockBuffers() returns leaf buffers from every clock tree. If nearby buffers belong to different clocks and their keyed parity requires a move, lines 239–241 disconnect a sequential clock pin from one domain and connect it to the other, changing the circuit's behavior; require both buffers to originate from the same clock before adding the candidate.
Useful? React with 👍 / 👎.
Comment on lines
+166
to
+168
| default: | ||
| // Vias carry no direction and are not part of planar wirelength. | ||
| break; |
There was a problem hiding this comment.
For any route that crosses a VIA or TECH_VIA, dbWireDecoder changes its current layer at the via, but this default branch leaves the local layer set to the pre-via layer. Consequently, the following segment is bucketed against the wrong preferred direction—usually reversing its classification on alternating-direction layers—and verifyRouting() can compute incorrect wrong-way fractions, p-values, and ownership verdicts.
Useful? React with 👍 / 👎.
Comment on lines
+128
to
+130
| const int dbu = block->getDbUnitsPerMicron(); | ||
| const std::int64_t max_dist | ||
| = static_cast<std::int64_t>(opts.sibling_dist_um) * dbu; |
There was a problem hiding this comment.
When -sibling_dist_um has a fractional value, converting it to int64_t before multiplying by DBU truncates the micron value first; for example, 0.5 becomes a zero-distance limit rather than 0.5 * dbu. Since the Tcl and C++ APIs deliberately expose this option as a double, fractional limits silently reject valid nearby buffer pairs.
Useful? React with 👍 / 👎.
Comment on lines
+77
to
+80
| set fh [open $path w] | ||
| # The file holds the secret key, so no one but its owner should be able to | ||
| # read it. Set the mode before anything is written to it. | ||
| catch { file attributes $path -permissions 0600 } |
There was a problem hiding this comment.
When the requested key path is on a filesystem where changing permissions fails, the catch discards that failure and the command proceeds to write the secret and all derived stage keys with the file's existing or default permissions. There is also a create-to-chmod window in which another local user can open a newly created permissive file and retain access to the subsequent writes; the command should fail rather than claim it produced an owner-only key file unless creation and permission hardening succeed safely.
Useful? React with 👍 / 👎.
maliberty
requested changes
Aug 18, 2026
| X(UTL) \ | ||
| X(WEB) | ||
| X(WEB) \ | ||
| X(WMK) |
Member
There was a problem hiding this comment.
Add to docs/contrib/DeveloperGuide.md ## Tool Flow Namespace
| int count = 0; | ||
| for (dbITerm* iterm : inst->getITerms()) { | ||
| if (iterm->getIoType() == odb::dbIoType::OUTPUT) { | ||
| ++count; |
Member
There was a problem hiding this comment.
Suggested change
| ++count; | |
| if (++count > 1) { | |
| break; | |
| } |
| std::ranges::transform(name, name.begin(), [](unsigned char c) { | ||
| return static_cast<char>(std::toupper(c)); | ||
| }); | ||
| return name == "CP" || name == "CLK" || name == "CK" || name == "CLOCK"; |
Member
There was a problem hiding this comment.
This question is better asked of sta than odb.
| { | ||
| std::vector<dbInst*> lcbs; | ||
| for (dbInst* inst : block->getInsts()) { | ||
| dbNet* out = singleOutputNet(inst); |
Member
There was a problem hiding this comment.
Flops can have Q and QB outputs.
| | `-key_hex` | 64-character hex placement key. | | ||
| | `-grid_nx`, `-grid_ny` | Tiles across and down the core, so marks are spread rather than clustered. Both default to `8`. | | ||
| | `-guard_degrade_ns` | Slack a pair may cost before it is put back. Defaults to `0.02`. `0` disables the check. | | ||
| | `-hpwl_eps_dbu` | Largest half-perimeter wirelength change a swap may cost, in database units. Defaults to `100`. | |
Member
There was a problem hiding this comment.
The user interface generally uses microns not dbu. This would also solve the problem of inconsistent default across PDKs.
Comment on lines
+578
to
+579
| gridCostVal = static_cast<frCost>(router_cfg_->GRIDCOST * edgeLength | ||
| * wrong_way_multiplier_); |
Member
There was a problem hiding this comment.
Why not
gridCostVal *= wrong_way_multiplier_;
|
|
||
| // Total length of the union of a set of intervals. Touching endpoints merge, | ||
| // so two records that meet end to end count once, not twice. | ||
| std::int64_t mergedLength(std::vector<std::pair<int, int>>& intervals) |
| have_prev = true; | ||
| break; | ||
| } | ||
| default: |
Member
There was a problem hiding this comment.
It is preferred to enumerate all cases and avoid default. That way if a new enum value is added it will be flagged here.
| layer = dec.getLayer(); | ||
| have_prev = true; // keep anchor at current (prev_x, prev_y) | ||
| break; | ||
| default: |
| // given routed net, in DBU. Vias contribute nothing (zero-length in | ||
| // the plane); PATH/POINT/POINT_EXT give rectilinear segments on the | ||
| // current layer. | ||
| void measureNetWirelength(dbNet* net, |
Member
There was a problem hiding this comment.
Similar to canonicalWirelength - could they be combined?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add the
wmkmodule for keyed physical-design watermarking and verification.The module implements PDMarks across three physical-design stages:
The implementation is primarily contained in
src/wmk. The DRT changes add a per-net watermark flag and apply the routing cost multiplier to tagged nets.See
src/wmk/README.mdfor the algorithm, command interface, claim format, and limitations.Type of Change
Impact
This PR adds watermark embedding and verification commands to OpenROAD.
The new functionality is inactive unless explicitly invoked. An unwatermarked flow follows the existing behavior.
The routing implementation adds a watermark flag to
frNetand a conditional cost multiplier inFlexGridGraph::getCostsfor tagged nets.Verification
./etc/Build.sh).The implementation includes:
clang-tidyverificationThe implementation has also been evaluated on several designs across NanGate45 and ASAP7, with zero DRC violations in the tested designs.
Related Issues
None