odb: factor out utilities for use in testing, start unit testing Ath__parser

[cdleary]

Signed-off-by: Christopher D. Leary cdleary@gmail.com

[maliberty]

/tmp/workspace/OpenROAD-Public_PR-2801-head/src/utl/include/utl/CFileUtils.h:35:10: fatal error: boost/core/span.hpp: No such file or directory
 #include <boost/core/span.hpp>

[cdleary]

/tmp/workspace/OpenROAD-Public_PR-2801-head/src/utl/include/utl/CFileUtils.h:35:10: fatal error: boost/core/span.hpp: No such file or directory
 #include <boost/core/span.hpp>

At first I was thinking the build configuration was incomplete somehow, but turns out the CentOS 7 build uses Boost 1.76.0 which doesn't have span in core: https://www.boost.org/doc/libs/1_76_0/libs/core/doc/html/index.html Will think about the best next step on Mon.

[maliberty]

I wouldn't overengineer something that is temporary scaffolding. Just pass a string or char* & length.

[cdleary]

I wouldn't overengineer something that is temporary scaffolding. Just pass a string or char* & length.

This may be a potato poh-tah-to thing, but I do think of this not as "overengineering this one unimportant thing" but more "how is modern C++ written, and can we use naturally-error-avoidant C++ conventions in the OpenROAD code base". Hopefully that makes sense -- if we were to ramp other engineers on the code base I'd want to be able to provide appropriate developer guidance/guidelines in that dimension.

[maliberty]

Modern c++ is not written with FILE*. This code should have its life span measured in months and nobody else should be using it.

[maliberty]

My view is that you should always prefer a std vector, array, or string to a raw C-style array unless very specifically justified by a substantial gain in profiling. This would be my guidance to new developer on the project.

We have legacy uses but I am not aware of any justified cases in OR so far. Therefore there haven't been any cases where span makes sense to me as a developer recommendation.

Recommendations generally live in https://github.com/The-OpenROAD-Project/OpenROAD/blob/master/docs/contrib/CodingPractices.md. Cherry wrote it and I generally agree. Practice #14 aligns with my comments above.

What is your feeling about when span should be used? Do you agree with the above?

[cdleary]

My view is that you should always prefer a std vector, array, or string to a raw C-style array unless very specifically justified by a substantial gain in profiling. This would be my guidance to new developer on the project.

Spans make code generic with respect to "we get contiguous elements in some form". They publish the minimal/essential requirement for the input data. E.g. if the values were allocated in a contiguous chunk in an area, you can refer to them with a span, though you could not with a const vector&. If you take a const vector& that mandates the representation of how the data gets enclosed, which is usually not relevant to algorithms, you just want to know the data is contiguous. This is the analog to modern C++ preferring to use string_view to taking a const string& (the latter of which forces the caller representation). Please see https://abseil.io/tips/93 and LMK what your thoughts are on this property.

I don't think I generally agree with the reasoning "because this PR contains FILE* we shouldn't care about discussing whether we can use modern constructs like span". I'm trying to understand how we write code in the OpenROAD code base. That means interop'ing with what's there (which in this case involves FILE*) while understanding how new code should be written in general. If we don't have a path to use spans, that's fine, but please do allow me to take a beat and understand if that's essential given the project's environmental constraints.

[cdleary]

Talking it over a bit with @QuantamHD -- we think if the CI for CentOS used the DependencyInstaller we would have a uniform version of Boost available. Right now it finds 1.76.0 in the system path, but the DependencyInstaller installs 1.80 (

OpenROAD/etc/DependencyInstaller.sh

Line 15 in 1a3a873

boostVersionBig=1.80

) -- this is quite similar to the TCL version skew issue we were seeing cause confusion last week -- from https://jenkins.openroad.tools/blue/rest/organizations/jenkins/pipelines/OpenROAD-Public/branches/PR-2801-head/runs/7/nodes/48/log/?start=0

-- Found SWIG: /usr/bin/swig (found suitable version "4.0.1", minimum required is "3.0")  
-- Found Boost: /usr/local/lib/cmake/Boost-1.76.0/BoostConfig.cmake (found version "1.76.0")  

Can we update the CI to do DependencyInstaller first and then we can assume a uniform boost library version (1.80)? That would enable use of spans and avoid more development-version-skew-based-issues going forward.

[maliberty]

@vvbandeira please comment on the boost version in CI.

[maliberty]

I'm glad to take a beat to discuss them. I have the same concerns about span and string_view as they serve a similar goal.

Swig doesn't support them so they can't be used in swig'ed interface. That means the recommendation on when to use them is more complex.

They are non-owning and therefore error prone. I understand that you get some efficiency benefit but I prefer safety as a first priority. The most common use cases to actually get a benefit from them are anti-patterns from my perspective (raw C style string or arrays).

Do you see practical benefit from them in your experience outside of legacy code support?

[cdleary]

I'm glad to take a beat to discuss them. I have the same concerns about span and string_view as they serve a similar goal.

Swig doesn't support them so they can't be used in swig'ed interface. That means the recommendation on when to use them is more complex.

I can mostly just speak to Google C++ code, but that Abseil C++ tip gives some of the reasons it's used pervasively here. Any time you have a "borrow" of a contiguous piece of data (mutable or immutable), a span (or const span) is used, because it avoids concretizing the code on an implementation detail that just makes it less reusable (e.g. taking a const vector&). The type itself documents that the lifetime of the data is borrowed from the caller, so people are less tempted to stash away a pointer to it for longer term keeping (as they might with a const vector&), as it also uses value semantics.

For bindings you can define a type map that makes defensive copies for returning outwards, similar to what you would do for a function returning a const vector&. Bindings always have to be more complicated when you return things with derivative lifetimes, spans are not particularly special there vs any other borrowed refs your functions might be returning.

They are non-owning and therefore error prone. I understand that you get some efficiency benefit but I prefer safety as a first priority. The most common use cases to actually get a benefit from them are anti-patterns from my perspective (raw C style string or arrays).

Do you see practical benefit from them in your experience outside of legacy code support?

Things like arenas, or being polymorphic over array vs vector are one use case. Another is that being able to pop off the prefix is O(1) which is not possible with a vector. As a case study, lexers/parsers classically take a view of contiguous characters and chomp off the front, and spans/views are the kind of things that give a natural datatype for safe bounds tracking and checking. Spans often /improve/ safety where people would normally reach for raw pointer/length values that then had to be externally maintained. One of the design goals for spans is that no performance is lost for these benefits.

Spans don't really have much to do with legacy code support, except that they are easy to make / reach for to make things safer if somebody was previously using a broken out pointer/length contiguous data idiom. The XLS project has no legacy code and it has 1.1k uses of span according to sourcegraph ( https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/google/xls%24+absl::Span+count:2000&patternType=standard&sm=0&groupBy=path ). Honestly it's one of those things you take for granted because it's a nice abstraction and usually not too contentious -- is there a particular bad property we'd be avoiding if we allowed use of boost spans (as are present in Boost 1.80, if we got uniform support for that)?

[cdleary]

@vvbandeira I was thinking we could use this PR as a canary for whether the dependencies were set up uniformly in CI (e.g. for Boost 1.80), but if that's not on your short list I'll just work around it in this PR. Let me know if you're thinking it's more "few days" or "few weeks", thanks!

[vvbandeira]

@vvbandeira I was thinking we could use this PR as a canary for whether the dependencies were set up uniformly in CI (e.g. for Boost 1.80), but if that's not on your short list I'll just work around it in this PR. Let me know if you're thinking it's more "few days" or "few weeks", thanks!

ETA tomorrow end of day, early Thursday the latest to have all CI synced.

[vvbandeira]

CI should be up to date now. You need to fix the merge conflict for CI to run. Also it is a good idea to merge latest master to get #2841, without it the CI will fail.

[cdleary]

CI should be up to date now. You need to fix the merge conflict for CI to run. Also it is a good idea to merge latest master to get #2841, without it the CI will fail.

Will do! Thanks for the update Vitor.

[cdleary]

One review comment note, I copy/pasta'd the unit test procedure from helpers.tcl in the RCX dir to the ODB one just because I'm not sure how imports work in the regression runner TCL setup. If you have guidance on how to avoid the copy/pasta would be happy to fix it.

[cdleary]

Just an explicit ping on that last request for comment @maliberty but no rush.

[maliberty]

There is already a version in test/helpers.tcl. I imagine the one in odb is left over from when it was an independent submodule and should be removed (along with your copy).

[cdleary]

There is already a version in test/helpers.tcl. I imagine the one in odb is left over from when it was an independent submodule and should be removed (along with your copy).

Ok turned helpers.tcl in the ODB subdir into a symlink to top level $OR/tests/helpers.tcl which required hoisting a few of the helpers that were depended upon in ODB tests. Hope that was what you were saying, PTAL.

[cdleary]

All comments addressed, but the Jenkins errors on some platforms is kind of curious. Looking...

[maliberty]

lgtm but I'll approve once the CI is resolved

[cdleary]

Just for posterity: one issue was that this was needed on the Centos7 build:

if {[package vcompare [package present Tcl] 8.6] == -1} {
    proc lmap {args} {
        set result {}
        set var [lindex $args 0]
        foreach item [lindex $args 1] {
            uplevel 1 "set $var $item"
            lappend result [uplevel 1 [lindex $args end]]
        }
        return $result
    }
}

[maliberty]

Just FYI, Ath__parser is another one on my list of things we should get rid of eventually.