search

The-OpenROAD-Project / OpenROAD

OpenROAD's unified application implementing an RTL-to-GDS Flow. Documentation at https://openroad.readthedocs.io/en/latest/
https://theopenroadproject.org/
BSD 3-Clause "New" or "Revised" License
1.53k stars 536 forks source link

OpenROAD regressions with boundary checks [-Wp,-D_GLIBCXX_ASSERTIONS] #1764

Closed cbalint13 closed 2 years ago

cbalint13 commented 2 years ago

There are no failing regressions on latest a1cd122fe9bf2e6cb7b2b05be43c50dc5599d8f2 (2022/04/10) unless boundary checks are enabled.

Suggestions:

Observations:

(x) Enabling boundary checks reveals these:

$ cat openroad-cxx-on.log | grep -e ERROR 
pd1 *ERROR* 
gcd_mem1_01 *ERROR* 
gcd_mem1_02 *ERROR* 
gcd_mem1_03 *ERROR* 
gcd_mem3_01 *ERROR* 
gcd_mem3_02 *ERROR* 
gcd_mem3_03 *ERROR* 
gcd_mem5_01 *ERROR* 
level3_01 *ERROR* 
level3_02 *ERROR* 
east_west1 *ERROR* 
east_west2 *ERROR* 
snap_layer1 *ERROR* 
repair_slew12 *ERROR* 
repair_antennas1 *ERROR* 
repair_antennas2 *ERROR* 
repair_antennas3 *ERROR* 
sw130_random *ERROR* 
sw130_random_simple *ERROR* 
gcd *ERROR* 
45_gcd *ERROR*

(x) All the above fails due to boundary issues, e.g:

$ cat src/rcx/test/results/45_gcd.log  | grep Asser
/usr/include/c++/12/bits/stl_vector.h:1122: std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>:
:operator[](size_type) [with _Tp = odb::tmg_rc; _Alloc = std::allocator<odb::tmg_rc>; 
reference = odb::tmg_rc&; size_type = long unsigned int]: Assertion '__n < this->size()' failed.

The documentation is way too summary, it covers more than enlisted there. A little sample (e.g. on a generic std::vector) on how boundary checks works:

$ cat test1.cpp 
#include <vector>
#include <cstdio>

int main()
{
    std::vector<int> v = { 7, 5, 16, 8 };
    printf("v[-1] = %i\n", v[-1]);
}
$ cat test2.cpp 
#include <vector>
#include <cstdio>

int main()
{
    std::vector<int> v = { 7, 5, 16, 8 };
    printf("v[4] = %i\n", v[4]);
}

It runs, doesn't fail:

$ c++ test1.cpp -o test1; ./test1
v[-1] = 0

$ c++ test2.cpp -o test2; ./test2
v[4] = 0

Now failures are guaranteed:

$ c++ test1.cpp -Wp,-D_GLIBCXX_ASSERTIONS -o test1; ./test1
/usr/include/c++/12/bits/stl_vector.h:1122: std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>
::operator[](size_type) [with _Tp = int; _Alloc = std::allocator<int>; reference = int&; 
size_type = long unsigned int]: Assertion '__n < this->size()' failed.
Aborted (core dumped)
$ c++ test2.cpp -Wp,-D_GLIBCXX_ASSERTIONS -o test2; ./test2
/usr/include/c++/12/bits/stl_vector.h:1122: std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>
::operator[](size_type) [with _Tp = int; _Alloc = std::allocator<int>; reference = int&; 
size_type = long unsigned int]: Assertion '__n < this->size()' failed.
Aborted (core dumped)

Cc @maliberty

Thanks, ~cristian.

maliberty commented 2 years ago

We should definitely fix the issues this flag catches. However it will have a runtime performance cost. Are you saying every release package has this flag on all the time, not just during development?

cbalint13 commented 2 years ago

We should definitely fix the issues this flag catches.

However it will have a runtime performance cost.

Are you saying every release package has this flag on all the time, not just during development?

Using it even in final releases allows catching new uncovered bugs from the very users.

Some more (offtopic) details:

Thanks, ~cristian.

maliberty commented 2 years ago

I've fixed a bunch of these (https://github.com/The-OpenROAD-Project/OpenROAD/pull/1765). The last all seem to come down the tmg* code which is really bad code in odb. Maybe its time to finally clean it up as fixing anything in there is excruciating.

proppy commented 2 years ago

@cbalint13 do you recommend that we also enable this for https://github.com/hdl/conda-eda/tree/master/pnr/openroad? maybe that could be something enable for CMake for all Release build?

cbalint13 commented 2 years ago

@cbalint13 do you recommend that we also enable this for https://github.com/hdl/conda-eda/tree/master/pnr/openroad? maybe that could be something enable for CMake for all Release build?

@proppy ,

You can for all else but not for openroad ,I suggest not yet, until all regression tests pass.

With it, openroad will fail on simple designs (e.g. the spm form openlane) at antenna-checking step. It's enabled everywhere else (in that repo) except the openroad package.

cbalint13 commented 2 years ago

@proppy ,

Double checked: all tools are fine, except cvc and openroad (opensta + abc are fine as external).

maliberty commented 2 years ago

All regressions have been fixed but I'm going to run all ORFS designs before closing this.

maliberty commented 2 years ago

@cbalint13 cvc issues should be filed at https://github.com/d-m-bailey/cvc

maliberty commented 2 years ago

All designs pass but some of the ISPD routing benchmarks have issues that need looking into.

cbalint13 commented 2 years ago

All designs pass but some of the ISPD routing benchmarks have issues that need looking into.

maliberty commented 2 years ago

Fixed with https://github.com/The-OpenROAD-Project/OpenROAD/pull/1777