Replace compat features with breaking changes #403

mandolaerik · 2025-12-05T23:11:22Z

No description provided.

mandolaerik · 2025-12-05T23:20:34Z

Remaining work:

update docs properly
I did some quick hackery to make existing if compat.xyz in dml.globals.enabled_compat checks work unchanged. I should actually go through these and re-express them as breaking changes. And revert some supporting code.

mandolaerik · 2025-12-05T23:22:19Z

py/dml/breaking_changes.py

+
+
+
+compat_features = {


once we eliminate uses of dml.globals.enabled_compat, this should be moved to its only use in dmlc.py

syssimics · 2025-12-05T23:37:34Z

PR Verification: ❌ failure

lwaern-intel · 2025-12-08T08:13:49Z

deprecations_to_md.py

 by_version = {}
-for feature in compat.features.values():
-    if (feature.last_api_version.str in api_versions()
+for feature in breaking_changes.changes.values():


Consider s/feature/change

lib/1.2/dml-builtins.dml

py/dml/breaking_changes.py

lwaern-intel · 2025-12-08T08:18:45Z

py/dml/breaking_changes.py

+    ```
+
+    > [!NOTE]
+    > When the change is disabled, proxy attributes are not created for all


py/dml/breaking_changes.py

lwaern-intel · 2025-12-08T08:31:29Z

py/dml/breaking_changes.py

+    '''
+    Up to Simics 7, DMLC's type checking was very lenient compared to GCC,
+    especially for method overrides and uses of `extern` macros. When this
+    change is enabled, type checking becomes strict and consistent with C.


More consistent with C. Dammit, I'm now realizing that I'll have to make another pass at this because you've made substantial changes to the documentation beyond just inverting the language used and saying "change" instead of "feature". That is going to tie up this PR, because you will be forced to defend the changes you've made to the documentation I've written. If you want to avoid that, please be less revisionist.

More consistent with C.

What I wanted to say is that it's consistent with how C code is generated.

Discussed verbally

lwaern-intel · 2025-12-08T08:38:48Z

py/dml/breaking_changes.py

+    indices used when calling the method were within bounds. When this change is
+    enabled, indices are implicitly range checked. If enabling this change causes
+    crashes, it indicates a bug in your model that could lead to memory
+    corruption without the range check.


Like here, see! You remove entirely the emphasis both in the short and long doc about just how bad failing the assert is and the implication to how users should enable the change even if we can't force them to.

I went through my changes, AFAICS I only removed significant chunks in two places. Moved removals to a separate commit.

The overall problem was that the docs were backward and headache-inducing, and flipping the polarity of some words did not help; it was better to rephrase everything.

lwaern-intel · 2025-12-08T08:40:29Z

py/dml/breaking_changes.py

+    typed parameters were inlined as constants, which had some
+    unintuitive semantic implications. In DML 1.4, constants are only
+    inlined in parameters declared using `inline` as quasi-type.  When
+    this change is enabled, DML 1.2 only inlining constants in untyped


DML 1.2 only inlines constants

lwaern-intel · 2025-12-08T08:41:05Z

py/dml/breaking_changes.py

+    provisional feature is explicitly enabled.
+
+    where the [`simics_util_vect` provisional feature](provisional-auto.html#simics_util_vect)
+    is not enabled.


stray artifact

lwaern-intel · 2025-12-08T08:42:34Z

py/dml/c_backend.py

            and port.objtype in {'port', 'bank'}
-            and compat.port_proxy_ifaces in dml.globals.enabled_compat):
+            and breaking_changes.remove_port_proxy_ifaces
+            not in dml.globals.enabled_breaking):


I've never liked how verbose these checks were with compat, and now we're making it worse. Consider adding an .enabled() helper function to the BreakingChange class.

Right, maybe we can move enabled_breaking to a class member of breaking_changes.BreakingChange; this pattern is not without precedence.

Factor out major-specific stuff to separate test, and rename latest_api_version to default_api_version. The idea is that the default API version is compatible with all major versions we care about, therefore we use that to run most tests; tests that rely on something newer can annotate that explicitly by e.g. `/// DMLC-FLAG --breaking-change`.

This lets us use whatever hardcoded isolated one-time hacks to preserve their legacy behaviour

Done in separate commit to make rename detection happy

mandolaerik · 2025-12-08T20:55:36Z

py/dml/c_backend.py

-            and (compat.dml12_misc not in dml.globals.enabled_compat
+            and (breaking_changes.remove_misc_quirks in dml.globals.enabled_breaking


I'm not sure it was right to auto-add the dml- prefix; maybe it's clearer and more greppable if we spell out the prefix. In this example, the 'dml12' part of dml12_misc_int effectively deemphasized the condition as extra uninteresting dml 1.2 junk.

mandolaerik · 2025-12-08T21:30:32Z

py/dml/breaking_changes.py

-    then that **definitely signifies a bug in your model; a bug that would
-    very likely result in memory corruption if the assertion were not to
-    be made.**'''
+    then that signifies a bug in your model that could
+    lead to memory corruption without the range check.'''


Changed to neutral language that says the same thing. The feature is important for those who spend much time on problems root-caused to memory corruptions, it's not up to us to decide. People are running sanitation tools that probably catch most such memory corruptions. IIRC, some of the overflows detected in the wild by the range check were not real corruptions (because it just logged something without accessing state). I.e., the code is inherently dangerous and unsafe but can't actually cause problems yet.

I agree it's a sane check that everyone should enable as soon as possible, but the same goes for most breaking changes. If we care, the best we can do is to actively push people to enable the check.

mandolaerik · 2025-12-08T21:35:43Z

py/dml/breaking_changes.py

-    Passing `--no-compat=suppress_WLOGMIXUP` to DMLC has almost the same effect
-    as passing `--warn=WLOGMIXUP`; either will cause DMLC to report the warning
-    even when the Simics API version in use is below 7. The only difference
-    between these two options is that if `--no-compat=suppress_WLOGMIXUP` is
-    used (and `--warn=WLOGMIXUP` is not), then `WLOGMIXUP` may still be
-    explicitly suppressed via `--no-warn=WLOGMIXUP`. In contrast,
-    `--warn=WLOGMIXUP` doesn't allow for `WLOGMIXUP` to be suppressed at
-    all.'''


Removed this because it's a lot of text and I don't understand why a user needs it. If they get the warning somewhere, then they will fix it. If that's impossible somewhere (which is unlikely), then they can use --no-warn as usual, no need to remind them.

mandolaerik · 2025-12-08T21:42:05Z

py/dml/breaking_changes.py

-    to avoid overwhelming users with warnings, as the faulty pattern that
-    `WLOGMIXUP` reports is very prevalent within existing code. Addressing
-    applications of the faulty pattern should be done before or as part of
-    migration to Simics API version 7.


Removed some text that felt redundant. It follows from how breaking changes work that WLOGMIXUP will be enabled during migration to API 7, and it follows from the previous mention of "overwhelming" that you want to fix those violations if you care about not getting overwhelmed.

syssimics · 2025-12-08T21:45:56Z

PR Verification: ❌ failure

This reverses polarity and makes things easier to explain

syssimics · 2025-12-09T21:29:46Z

PR Verification: ❌ failure

lwaern-intel · 2025-12-10T09:47:22Z

py/dml/breaking_changes.py

+    def enabled(self):
+        # `None` happens in unit tests; assume everything is enabled
+        return (True if BreakingChange.enabled_breaking_changes is None
+                else self in BreakingChange.enabled_breaking_changes)


(BreakingChange.enabled_breaking_changes is None || self in BreakingChange.enabled_breaking_changes)

lwaern-intel · 2025-12-10T09:50:45Z

py/dml/breaking_changes.py

+
+
+@change
+class dml_remove_legacy_attributes(BreakingChange):


It's replacement, not pure removal. Use no_legacy_attributes or modern_attributes.

It is pure removal. Before the change you get both the normal attr and a proxy pseudo-attr; the only change is that the proxy is no longer added.

This is not the proxy attributes removal breaking change. This is the SIM_register_attribute breaking change.

lwaern-intel · 2025-12-10T09:51:51Z

py/dml/breaking_changes.py

+
+
+@change
+class dml_remove_port_obj_param(BreakingChange):


this is a rather terrible name which when read could be understood as the exact opposite of what the feature does

py/dml/breaking_changes.py

lwaern-intel · 2025-12-10T10:12:37Z

py/dml/breaking_changes.py

-    <tt>uint<i>NN</i></tt> types; this can sometimes have unexpected
-    consequences. The most immediate effect of disabling this feature
+@change
+class dml12_fix_int_quirks(BreakingChange):


Fix int quirks? That's a strange way of putting it, when the intent is rather to strip away strange legacy behaviors. Not blocking, but I encourage you to give naming another think.

lwaern-intel · 2025-12-10T10:13:04Z

py/dml/breaking_changes.py

-    `experimental_vect` is disabled, DMLC forbids the `vect`
-    syntax.
+@change
+class dml_forbid_experimental_vect(BreakingChange):


ban_ or no_. In general, I'm sensitive to the length of these names, especially now we slap on the dml prefix on everything...

maybe vect_requires_provisional?

I like that. Or rather, vect_needs_provisional.

no wait that won't make sense once we get RAII. Uhh... legacy_vect_needs_provisional? but augh now it's long again

no_implicit_simics_util_vect?????

oh dammit just pick no_experimental_vect

lwaern-intel · 2025-12-10T10:25:52Z

py/dml/breaking_changes.py

+    '''
+    Up to Simics 7, DMLC's type checking was very lenient compared to GCC,
+    especially for method overrides and uses of `extern` macros. When this
+    change is enabled, type checking becomes strict and consistent with C.


Discussed verbally

mandolaerik requested review from jhbaarnh and lwaern-intel and removed request for lwaern-intel December 5, 2025 23:11

mandolaerik marked this pull request as draft December 5, 2025 23:12

mandolaerik commented Dec 5, 2025

View reviewed changes

mandolaerik force-pushed the pr/replace-compat-features-with-breaking-changes branch from 3ccafbc to 85fff3c Compare December 6, 2025 10:34

lwaern-intel requested changes Dec 8, 2025

View reviewed changes

mandolaerik added 8 commits December 8, 2025 14:13

Add API-VERSION annotation

d3b430a

Remove dead code

ab05321

Only run tests with matching API version

89b6926

Utilize API-VERSION annotation

0f5e4a3

Add tests for all _compat_* params

97788dc

This lets us use whatever hardcoded isolated one-time hacks to preserve their legacy behaviour

Rename compat.py to breaking_changes.py

d1453fd

Done in separate commit to make rename detection happy

Remove some text with limited usefulness

6b71e20

mandolaerik commented Dec 8, 2025

View reviewed changes

mandolaerik force-pushed the pr/replace-compat-features-with-breaking-changes branch from 85fff3c to a808cba Compare December 8, 2025 21:21

syssimics requested a review from lwaern-intel December 8, 2025 21:21

mandolaerik commented Dec 8, 2025

View reviewed changes

mandolaerik force-pushed the pr/replace-compat-features-with-breaking-changes branch from a808cba to a56fb0b Compare December 9, 2025 06:55

mandolaerik added 4 commits December 9, 2025 14:46

Rework compat features into breaking changes

d8f7bb6

This reverses polarity and makes things easier to explain

Generate params for all breaking changes

2c0f959

Update uses of enabled_compat to use breaking_changes instead

0e3e906

Isolate "compat feature" support to dmlc.py

23d8793

mandolaerik force-pushed the pr/replace-compat-features-with-breaking-changes branch from a56fb0b to 23d8793 Compare December 9, 2025 21:06

lwaern-intel requested changes Dec 10, 2025

View reviewed changes

		and (compat.dml12_misc not in dml.globals.enabled_compat
		and (breaking_changes.remove_misc_quirks in dml.globals.enabled_breaking




		compat_features = {

Replace compat features with breaking changes #403

Are you sure you want to change the base?

Replace compat features with breaking changes #403

Uh oh!

Conversation

mandolaerik commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mandolaerik commented Dec 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syssimics commented Dec 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syssimics commented Dec 8, 2025

Uh oh!

syssimics commented Dec 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lwaern-intel Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lwaern-intel Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lwaern-intel Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mandolaerik commented Dec 5, 2025 •

edited

Loading

lwaern-intel Dec 10, 2025 •

edited

Loading

lwaern-intel Dec 11, 2025 •

edited

Loading

lwaern-intel Dec 11, 2025 •

edited

Loading