rsz: Source `StaTclTypes.i` and clean up the binding

[povik]

To get all the SWIG type conversion rules around basic STA types, it's best if we can source StaTclTypes.i. Here we make the change in the resizer binding, and use it to clean up some of the binding code. See the commits.

[povik]

The following STA patch is required, otherwise we get a conflict at linkage time. Please confirm the resizer change is desirable, I can open the OpenSTA PR for it.

diff --git a/tcl/StaTclTypes.i b/tcl/StaTclTypes.i
index 6127dea..abe3ca6 100644
--- a/tcl/StaTclTypes.i
+++ b/tcl/StaTclTypes.i
@@ -154,7 +154,7 @@ tclListNetworkSet(Tcl_Obj *const source,
     return nullptr;
 }
 
-StringSet *
+static StringSet *
 tclListSetConstChar(Tcl_Obj *const source,
 		    Tcl_Interp *interp)
 {
@@ -174,7 +174,7 @@ tclListSetConstChar(Tcl_Obj *const source,
     return nullptr;
 }
 
-StringSeq *
+static StringSeq *
 tclListSeqConstChar(Tcl_Obj *const source,
 		    Tcl_Interp *interp)
 {
@@ -194,7 +194,7 @@ tclListSeqConstChar(Tcl_Obj *const source,
     return nullptr;
 }
 
-StdStringSet *
+static StdStringSet *
 tclListSetStdString(Tcl_Obj *const source,
 		    Tcl_Interp *interp)
 {
@@ -279,7 +279,7 @@ setPtrTclList(SET_TYPE *set,
 
 ////////////////////////////////////////////////////////////////
 
-void
+static void
 tclArgError(Tcl_Interp *interp,
             const char *msg,
             const char *arg)
@@ -292,7 +292,7 @@ tclArgError(Tcl_Interp *interp,
   stringDelete(error);
 }
 
-void
+static void
 objectListNext(const char *list,
 	       const char *type,
 	       // Return values.

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[povik]

As a result of the new include, and with the static functions STA patch, we get new warnings

[ 97%] Building CXX object src/rsz/src/CMakeFiles/rsz_swig.dir/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx.o
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:1926:1: warning: unused function 'tclListSetConstChar' [-Wunused-function]
tclListSetConstChar(Tcl_Obj *const source,
^
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:1946:1: warning: unused function 'tclListSeqConstChar' [-Wunused-function]
tclListSeqConstChar(Tcl_Obj *const source,
^
...

If both openroad/opensta used C++17 we could silence with [[maybe_unused]] but I don't think that's the case.

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[gadfort]

@povik is it possible to use %import instead of %include to avoid the need for the sta patch?

[povik]

is it possible to use %import instead of %include to avoid the need for the sta patch?

@gadfort changing that and reverting the sta patch I get the failures below. I am not familiar with SWIG so it's hard for me to say how much work it would take to make %import work.

/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:3096:12: error: use of undeclared identifier 'tclListSeqPtr'
    arg2 = tclListSeqPtr<const Instance*>(objv[2], SWIGTYPE_p_Instance, interp);
           ^
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:3126:12: error: use of undeclared identifier 'tclListSeqPtr'
    arg1 = tclListSeqPtr<const Instance*>(objv[1], SWIGTYPE_p_Instance, interp);
           ^
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:3582:5: error: use of undeclared identifier 'seqPtrTclList'
    seqPtrTclList<NetSeq, Net>(nets, SWIGTYPE_p_Net, interp);
    ^
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:3613:5: error: use of undeclared identifier 'setPtrTclList'
    setPtrTclList<PinSet, Pin>(pins, SWIGTYPE_p_Pin, interp);
    ^
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:4371:5: error: use of undeclared identifier 'seqPtrTclList'
    seqPtrTclList<NetSeq, Net>(result, SWIGTYPE_p_Net, interp);
    ^
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:4657:24: error: use of undeclared identifier 'cmdNetwork'
    Network *network = cmdNetwork();
                       ^
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:4658:12: error: use of undeclared identifier 'tclListNetworkSet'
    arg1 = tclListNetworkSet<PinSet, Pin>(objv[1], SWIGTYPE_p_Pin, interp, network);
           ^
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:4676:5: error: use of undeclared identifier 'setTclList'
    setTclList<PinSet, Pin>(result, SWIGTYPE_p_Pin, interp);
    ^

[gadfort]

@povik thanks for checking, I was hoping that would have solved it.

[maliberty]

I appears %import doesn't bring the template functions into scope and %include leads to duplication. Using static works at the minor cost of having duplicate instantiations in the program.

A slightly nicer patch would be to pull these helper functions out of the .i into a regular c++ where they could more easily be shared. I believe then %import could be made to work. I'm not sure its worth the bother though.

[povik]

@maliberty Should I open a PR against OpenROAD's or upstream STA?

[maliberty]

@maliberty Should I open a PR against OpenROAD's or upstream STA?

upstream (which requires a CLA)

[povik]

parallaxsw/OpenSTA#62, which was blocking this PR, has now been merged

[povik]

Rebased and inserted sign-offs, this could be good to go now

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[povik]

The CI failures are due to some of the helpers included from StaTclTypes.i ending up not getting used in Resizer.i (but marked static):

/tmp/workspace/OpenROAD-Public_PR-5277-merge/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:2132:1: error: 'void sta::objectListNext(const char*, const char*, bool&, const char*&)' defined but not used [-Werror=unused-function]
 2132 | objectListNext(const char *list,
      | ^~~~~~~~~~~~~~
/tmp/workspace/OpenROAD-Public_PR-5277-merge/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:2119:1: error: 'void sta::tclArgError(Tcl_Interp*, const char*, const char*)' defined but not used [-Werror=unused-function]
 2119 | tclArgError(Tcl_Interp *interp,
      | ^~~~~~~~~~~
/tmp/workspace/OpenROAD-Public_PR-5277-merge/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:2012:1: error: 'sta::StringSeq* sta::tclListSeqConstChar(Tcl_Obj*, Tcl_Interp*)' defined but not used [-Werror=unused-function]
 2012 | tclListSeqConstChar(Tcl_Obj *const source,
      | ^~~~~~~~~~~~~~~~~~~
/tmp/workspace/OpenROAD-Public_PR-5277-merge/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:1992:1: error: 'sta::StringSet* sta::tclListSetConstChar(Tcl_Obj*, Tcl_Interp*)' defined but not used [-Werror=unused-function]
 1992 | tclListSetConstChar(Tcl_Obj *const source,
      | ^~~~~~~~~~~~~~~~~~~

Should I open another STA patch to have [[maybe_unused]] on those helpers? That sounds the cleanest, though I am not sure OpenSTA is C++17.

[maliberty]

Looks like it uses c++17 https://github.com/parallaxsw/OpenSTA/blob/bd13bc409ebac682aa25778f770458829ff14cc2/CMakeLists.txt#L565 so you can try a PR there

[povik]

The CI failures are due to some of the helpers included from StaTclTypes.i ending up not getting used in Resizer.i (but marked static):

This could have been resolved by https://github.com/parallaxsw/OpenSTA/pull/182

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"