add tests, unify odb error reporting

Changelog.md

@@ -23,10 +23,26 @@
   * Changed default value of `HOLD_VIOLATION_CORNERS` to `['*']`, which will
     raise an error for hold violations on *any* corners.
 
+* `Odb.*`
+
+  * Unified error handling with that of OpenROAD steps, i.e., dependent on the
+    `[ERROR (code)]` alerts.
+  * Metrics emitted from Odb steps are now also aggregated.
+  * **API**: instance variable `.alerts` now holds emitted alerts until the next
+    `start()`, similar to `.state_out`.
+
+* Created `Odb.InsertECOBuffer`, `Odb.InsertECODiode`
+
+  * New ECO steps using the variables `INSERT_ECO_BUFFERS` and
+    `INSERT_ECO_DIODES` respectively to allow creation of buffers and diodes
+    after (and only after global routing,) with an option to run it after
+    detailed routing so long as detailed routing is run again afterwards.
+
 * `Odb.AddPDNObstructions`, `Odb.AddRoutingObstructions`
 
   * `PDN_OBSTRUCTIONS` and `ROUTING_OBSTRUCTIONS` are now lists of tuples
     instead of variable-length Tcl-style lists (AKA: strings).
+  * **Internal**: Unified exit codes.
 
 * `Odb.DiodesOnPorts`, `Odb.PortDiodePlacement`
   * Steps no longer assume `DIODE_CELL` exists and fall back to doing nothing.
@@ -49,6 +65,8 @@
   * Always read libs before reading odb.
   * Added `log_cmd` from OpenROAD-flow-scripts -- neat idea for consistency
   * Lib files are now *always* read BEFORE reading database files.
+  * **API**: instance variable `.alerts` now holds emitted alerts until the next
+    `start()`, similar to `.state_out`.
   * **Internal**: Steps now sensitive to `_OPENROAD_GUI` environment variable --
     coupled with `--only`, it runs a step in OpenROAD then doesn't quit so you
     may inspect the result.

openlane/examples/spm/config.yaml

@@ -31,3 +31,24 @@ pdk::gf180mcu*:
   FP_CORE_UTIL: 40
   MAX_FANOUT_CONSTRAINT: 4
   PL_TARGET_DENSITY: 0.5
+
+INSERT_ECO_BUFFERS:
+  - target: _102_/A1
+    buffer: sky130_fd_sc_hd__clkbuf_2
+    placement: [74, 81.3]
+  - target: _102_/A2
+    buffer: sky130_fd_sc_hd__clkbuf_2
+    placement: [81, 81.3]
+
+INSERT_ECO_DIODES:
+  - target: _102_/A1
+    placement: [74, 81.3]
+  - target: _102_/A2
+    placement: [81, 81.3]
+
+meta:
+  flow: Classic
+  substituting_steps:
+    +OpenROAD.DetailedRouting: Odb.InsertECODiodes
+    +Odb.InsertECODiodes: Odb.InsertECOBuffers
+    +Odb.InsertECOBuffers: OpenROAD.DetailedRouting

openlane/scripts/odbpy/defutil.py

@@ -21,7 +21,6 @@
 
 from reader import click_odb, click
 from typing import Tuple, List
-from exception_codes import METAL_LAYER_ERROR, FORMAT_ERROR, NOT_FOUND_ERROR
 
 
 @click.group()
@@ -481,7 +480,7 @@ def parse_obstructions(obstructions) -> List[Tuple[str, List[int]]]:
                 f"[ERROR] Incorrectly formatted input {obs}.\n Format: layer llx lly urx ury, ...",
                 file=sys.stderr,
             )
-            sys.exit(FORMAT_ERROR)
+            sys.exit(1)
         else:
             layer = m.group("layer")
             bbox = [Decimal(x) for x in m.group("bbox").split()]
@@ -505,8 +504,8 @@ def add_obstructions(reader, input_lefs, obstructions):
         layer = obs[0]
         odb_layer = reader.tech.findLayer(layer)
         if odb_layer is None:
-            print(f"[ERROR] layer {layer} doesn't exist.", file=sys.stderr)
-            sys.exit(METAL_LAYER_ERROR)
+            print(f"[ERROR] Layer '{layer}' not found.", file=sys.stderr)
+            sys.exit(1)
         bbox = obs[1]
         dbu = reader.tech.getDbUnitsPerMicron()
         bbox = [int(x * dbu) for x in bbox]
@@ -550,8 +549,8 @@ def remove_obstructions(reader, input_lefs, obstructions):
         bbox = [int(x * dbu) for x in bbox]  # To dbus
         found = False
         if reader.tech.findLayer(layer) is None:
-            print(f"[ERROR] layer {layer} doesn't exist.", file=sys.stderr)
-            sys.exit(METAL_LAYER_ERROR)
+            print(f"[ERROR] Layer '{layer}' not found.", file=sys.stderr)
+            sys.exit(1)
         for odb_obstruction in existing_obstructions:
             odb_layer, odb_bbox, odb_obj = odb_obstruction
             if (odb_layer, odb_bbox) == (layer, bbox):
@@ -565,7 +564,7 @@ def remove_obstructions(reader, input_lefs, obstructions):
                 f"[ERROR] Obstruction on {layer} at {bbox} (DBU) not found.",
                 file=sys.stderr,
             )
-            sys.exit(NOT_FOUND_ERROR)
+            sys.exit(1)
 
 
 cli.add_command(remove_obstructions)

openlane/scripts/odbpy/eco_buffer.py

@@ -43,21 +43,21 @@ def cli(reader):
 
         target = reader.block.findInst(name_escaped)
         if target is None:
-            print(f"[ERROR] Instance '{target_name}' does not exist.", file=sys.stderr)
+            print(f"[ERROR] Instance '{target_name}' not found.", file=sys.stderr)
             exit(-1)
 
         master = reader.db.findMaster(buffer_master)
         if master is None:
             print(
-                f"[ERROR] Buffer type '{buffer_master}' does not exist.",
+                f"[ERROR] Buffer type '{buffer_master}' not found.",
                 file=sys.stderr,
             )
             exit(-1)
 
         target_iterm = target.findITerm(target_pin)
         if target_iterm is None:
             print(
-                f"[ERROR] Pin '{target_pin}' does not exist on instance {target_name}.",
+                f"[ERROR] Pin '{target_pin}' not found for instance '{target_name}'.",
                 file=sys.stderr,
             )
             exit(-1)
@@ -131,9 +131,9 @@ def cli(reader):
         target_iterm.connect(eco_net)
 
         if target_info.get("placement") is not None:
-            eco_loc = target_info["placement"]
-            eco_x = reader.block.micronsToDbu(eco_loc[0])  # convert to database units
-            eco_y = reader.block.micronsToDbu(eco_loc[1])  # convert to database units
+            eco_x, eco_y = target_info["placement"]
+            eco_x = reader.block.micronsToDbu(float(eco_x))
+            eco_y = reader.block.micronsToDbu(float(eco_y))
             eco_loc = (eco_x, eco_y)
         else:
             driver_loc = target.getLocation()
@@ -173,7 +173,6 @@ def cli(reader):
     dpl.detailedPlacement(max_disp_x, max_disp_y)
 
     grt_inc.updateRoutes(True)
-    grt.saveGuides()
 
     for inst, previous_status in insts_to_temporarily_lock_then_unlock_later:
         inst.setPlacementStatus(previous_status)

openlane/scripts/odbpy/eco_diode.py

@@ -61,7 +61,7 @@ def cli(reader):
         target_iterm = target.findITerm(target_pin)
         if target_iterm is None:
             print(
-                f"[ERROR] Pin '{target_pin}' on instance '{target_name}' not found.",
+                f"[ERROR] Pin '{target_pin}' not found for instance '{target_name}'.",
                 file=sys.stderr,
             )
             exit(-1)
@@ -97,16 +97,10 @@ def cli(reader):
 
         if target_info["placement"] is not None:
             x, y = target_info["placement"]
-            x = reader.block.micronsToDbu(x)
-            y = reader.block.micronsToDbu(y)
+            x = reader.block.micronsToDbu(float(x))
+            y = reader.block.micronsToDbu(float(y))
         else:
             x, y = target.getLocation()
-            if x is None or y is None:  # Check if getLocation returns None
-                print(
-                    f"[ERROR] Could not get location for instance '{target_name}'.",
-                    file=sys.stderr,
-                )
-                exit(-1)
 
         eco_diode.setOrient("R0")
         eco_diode.setLocation(x, y)
@@ -127,7 +121,6 @@ def cli(reader):
     dpl.detailedPlacement(max_disp_x, max_disp_y)
 
     grt_inc.updateRoutes(True)
-    grt.saveGuides()
 
     for inst, previous_status in insts_to_temporarily_lock_then_unlock_later:
         inst.setPlacementStatus(previous_status)

openlane/steps/odb.py

@@ -22,7 +22,7 @@
 from dataclasses import dataclass
 from typing import Dict, List, Literal, Optional, Tuple
 
-from ..common import Path, get_script_dir
+from ..common import Path, get_script_dir, aggregate_metrics
 from ..config import Instance, Macro, Variable
 from ..logging import info, verbose
 from ..state import DesignFormat, State
@@ -35,6 +35,7 @@
     DefaultOutputProcessor,
     MetricsUpdate,
     Step,
+    StepError,
     StepException,
     ViewsUpdate,
 )
@@ -49,6 +50,8 @@ class OdbpyStep(Step):
 
     output_processors = [OpenROADOutputProcessor, DefaultOutputProcessor]
 
+    alerts: Optional[List[OpenROADAlert]] = None
+
     def on_alert(self, alert: OpenROADAlert) -> OpenROADAlert:
         if alert.code in [
             "ORD-0039",  # .openroad ignored with -python
@@ -62,6 +65,8 @@ def on_alert(self, alert: OpenROADAlert) -> OpenROADAlert:
         return alert
 
     def run(self, state_in: State, **kwargs) -> Tuple[ViewsUpdate, MetricsUpdate]:
+        self.alerts = None
+
         kwargs, env = self.extract_env(kwargs)
 
         automatic_outputs = set(self.outputs).intersection(
@@ -84,25 +89,47 @@ def run(self, state_in: State, **kwargs) -> Tuple[ViewsUpdate, MetricsUpdate]:
         env["PYTHONPATH"] = (
             f'{os.path.join(get_script_dir(), "odbpy")}:{env.get("PYTHONPATH")}'
         )
+        check = False
+        if "check" in kwargs:
+            check = kwargs.pop("check")
 
         subprocess_result = self.run_subprocess(
             command,
             env=env,
+            check=check,
             **kwargs,
         )
+        generated_metrics = subprocess_result["generated_metrics"]
 
+        # 1. Parse warnings and errors
+        self.alerts = subprocess_result.get("openroad_alerts") or []
+        if subprocess_result["returncode"] != 0:
+            error_strings = [
+                str(alert) for alert in self.alerts if alert.cls == "error"
+            ]
+            if len(error_strings):
+                error_string = "\n".join(error_strings)
+                raise StepError(
+                    f"{self.id} failed with the following errors:\n{error_string}"
+                )
+            else:
+                raise StepException(
+                    f"{self.id} failed unexpectedly. Please check the logs and file an issue."
+                )
+        # 2. Metrics
         metrics_path = os.path.join(self.step_dir, "or_metrics_out.json")
-        metrics_updates: MetricsUpdate = subprocess_result["generated_metrics"]
         if os.path.exists(metrics_path):
             or_metrics_out = json.loads(open(metrics_path).read(), parse_float=Decimal)
             for key, value in or_metrics_out.items():
                 if value == "Infinity":
                     or_metrics_out[key] = inf
                 elif value == "-Infinity":
                     or_metrics_out[key] = -inf
-            metrics_updates.update(or_metrics_out)
+            generated_metrics.update(or_metrics_out)
 
-        return views_updates, metrics_updates
+        metric_updates_with_aggregates = aggregate_metrics(generated_metrics)
+
+        return views_updates, metric_updates_with_aggregates
 
     def get_command(self) -> List[str]:
         metrics_path = os.path.join(self.step_dir, "or_metrics_out.json")

openlane/steps/openroad.py

@@ -191,6 +191,8 @@ class OpenROADStep(TclStep):
 
     output_processors = [OpenROADOutputProcessor, DefaultOutputProcessor]
 
+    alerts: Optional[List[OpenROADAlert]] = None
+
     config_vars = [
         Variable(
             "PNR_CORNERS",
@@ -301,6 +303,7 @@ def run(self, state_in, **kwargs) -> Tuple[ViewsUpdate, MetricsUpdate]:
         2. After the `super()` call: Processes the `or_metrics_out.json` file and
         updates the State's `metrics` property with any new metrics in that object.
         """
+        self.alerts = None
         kwargs, env = self.extract_env(kwargs)
         env = self.prepare_env(env, state_in)
 
@@ -373,9 +376,11 @@ def run(self, state_in, **kwargs) -> Tuple[ViewsUpdate, MetricsUpdate]:
             views_updates[output] = path
 
         # 1. Parse warnings and errors
-        alerts = subprocess_result["openroad_alerts"]
+        self.alerts = subprocess_result.get("openroad_alerts") or []
         if subprocess_result["returncode"] != 0:
-            error_strings = [str(alert) for alert in alerts if alert.cls == "error"]
+            error_strings = [
+                str(alert) for alert in self.alerts if alert.cls == "error"
+            ]
             if len(error_strings):
                 error_string = "\n".join(error_strings)
                 raise StepError(

test/steps/conftest.py

@@ -22,13 +22,14 @@
 
 def collect_step_tests():
     excluded_tests = set()
-    for line in open(pytest.step_excl_dir, encoding="utf8"):
-        if match := entry.match(line):
-            line = match[1]
-        line = line.strip()
-        if line == "":
-            continue
-        excluded_tests.add(line)
+    with open(pytest.step_excl_dir, encoding="utf8") as f:
+        for line in f:
+            if match := entry.match(line):
+                line = match[1]
+            line = line.strip()
+            if line == "":
+                continue
+            excluded_tests.add(line)
     result = []
     test_rx = re.compile(r"\d+\-[A-Za-z\d_]+")
     for p in sorted(