Skip to content

odb: Replace CDL <> (normal and escaped) to [] to match master term name when exporting CDL file. #8433

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Oct 9, 2025
Merged

odb: Replace CDL <> (normal and escaped) to [] to match master term name when exporting CDL file. #8433

merged 2 commits into from
Oct 9, 2025

Conversation

@titan73
Copy link
Contributor

@titan73 titan73 commented Sep 25, 2025

Issue seen with:
[WARNING ODB-0286] Terminal OP_REG<2> of CDL master NVM_IP not found in LEF.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 25, 2025

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

@titan73
Copy link
Contributor Author

titan73 commented Sep 25, 2025

Lint Bazel fails with:
Error: Process completed with exit code 4.

But I can't see any error that could cause this in the log.

gadfort
gadfort reviewed Sep 25, 2025
View reviewed changes
Copy link
Collaborator

@gadfort gadfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a test

src/odb/src/cdl/cdl.cpp Outdated
Comment on lines 133 to 134
std::replace(token.begin(), token.end(), '<', '[');
std::replace(token.begin(), token.end(), '>', ']');
Copy link
Collaborator

@gadfort gadfort Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if these are escaped? This seems like it would just replace them.

Copy link
Contributor Author

@titan73 titan73 Sep 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "these"? The < and > chars? So \< and \>?

Copy link
Collaborator

@gadfort gadfort Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are correct the \< and \>

Copy link
Contributor Author

@titan73 titan73 Sep 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this escaping exists for CDL? I've never seen it.

Copy link
Member

@maliberty maliberty Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gadfort is this resolved?

@maliberty
Copy link
Member

maliberty commented Sep 25, 2025

You can ignore Lint Bazel for now.

@titan73 titan73 changed the title odb: Replace CDL <> to [] to match master term name when exporting CDL file. odb: Replace CDL <> (normal and escaped) to [] to match master term name when exporting CDL file. Oct 2, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2025

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

@titan73
Copy link
Contributor Author

titan73 commented Oct 2, 2025

Added test and handle escaped chars.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2025

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

@titan73
Copy link
Contributor Author

titan73 commented Oct 2, 2025 •
edited
Loading

The added test is failing with Bazelisk:

17:58:36  //src/odb/test:write_cdl-tcl_test                                        FAILED in 0.4s
17:58:36    /root/.cache/bazel/_bazel_root/4bdb15269cb0d44cbe6a5a164d220885/execroot/_main/bazel-out/k8-fastbuild/testlogs/src/odb/test/write_cdl-tcl_test/test.log
17:58:36  
17:58:36  Executed 1069 out of 1095 tests: 1094 tests pass and 1 fails locally.

I don't know anything about bazel. What's the way to fix the failure?

@maliberty
Copy link
Member

maliberty commented Oct 2, 2025

I don't know anything about bazel. What's the way to fix the failure?

I just pushed a fix. By default a test can only access <test_name>.* and write_cdl needed access to write_cdl_escaped.cdl

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2025

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

@titan73
Copy link
Contributor Author

titan73 commented Oct 3, 2025 •
edited
Loading

I just pushed a fix. By default a test can only access <test_name>.* and write_cdl needed access to write_cdl_escaped.cdl

Ok Thanks.

Not related to this PR but cts.skip_nets.tcl test is failing in pr-merge.

@maliberty
Copy link
Member

maliberty commented Oct 3, 2025

skip_nets is fixed in master now. I don't see any other failure.

@maliberty maliberty requested a review from gadfort October 3, 2025 05:16
@titan73
Copy link
Contributor Author

titan73 commented Oct 7, 2025 •
edited
Loading

@gadfort Any feedback on this PR?

gadfort
gadfort requested changes Oct 9, 2025
View reviewed changes
Copy link
Collaborator

@gadfort gadfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, but the test needs to be fixed.

src/odb/test/write_cdl.tcl Outdated
Comment on lines 8 to 10
write_cdl -masters {write_cdl.cdl} results/write_cdl_out.cdl

write_cdl -masters {write_cdl_escaped.cdl} results/write_cdl_escaped_out.cdl
Copy link
Collaborator

@gadfort gadfort Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test doesn't actually check anything.
You will need to diff the output file to ensure the output stays valid:
See

OpenROAD/src/pdn/test/asap7_macro_notcovered.tcl

Line 31 in 22a4b4c

diff_files asap7_macro_notcovered.defok $def_file

Copy link
Contributor Author

@titan73 titan73 Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks

@titan73
odb: Replace CDL <> (normal and escaped) to [] to match master term n…
2b29b89 
…ame when exporting CDL file.

Signed-off-by: Christian COSTA <[email protected]>
@titan73 titan73 requested a review from gadfort October 9, 2025 15:59
@titan73
Copy link
Contributor Author

titan73 commented Oct 9, 2025

Also unrelated test failing:
18:05:32 (18:05:32) ERROR: Analysis of target '//test/orfs/mock-array:MockArray_8x8_flat_power_instances_final_test' failed; build aborted: Analysis failed

@maliberty maliberty merged commit d4336ae into The-OpenROAD-Project:master Oct 9, 2025
11 of 14 checks passed
@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maliberty maliberty maliberty approved these changes

@gadfort gadfort gadfort approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@titan73 @maliberty @gadfort