Conversation
|
What variation in output did we see due to nondeterministic iteration? Was it reordered output definitions or something more subtle/messy? Definitely worthwhile to make our output deterministic here, I'm just curious how this manifested. |
kkysen
left a comment
There was a problem hiding this comment.
What variation in output did we see due to nondeterministic iteration? Was it reordered output definitions or something more subtle/messy? Definitely worthwhile to make our output deterministic here, I'm just curious how this manifested.
It affected the order of macro expansions that we folded over, which ended up meaning sometimes a macro expansion would be replaced by the macro definition, but not always.
| } | ||
| pub type zstd_platform_dependent_type = std::ffi::c_long; | ||
| pub const NESTED_INT: std::ffi::c_int = 0xffff as std::ffi::c_int; | ||
| pub const true_0: std::ffi::c_int = unsafe { 1 as std::ffi::c_int }; |
There was a problem hiding this comment.
@fw-immunant, note that this does achieve a lot of what #290 does. It's similar to the portable types, stuff. There, we'll now emit a uint32_t type alias instead of a u32 directly, and this will emit a true_0 const instead of a true literal directly. Both could then be refactored pretty easily. I'm not exactly sure how #290 works, but the advantage here is that it matches where the C used the true and false macros, and if C uses 0 and non-zero directly, then it keeps that behavior. And then I'm not sure how #290 handles ints vs _Bools either.
We can also resurrect #295 to use r#true raw identifiers instead if that's helpful.
There was a problem hiding this comment.
There definitely is some overlap in spirit but I think it's probably worthwhile to address this from both sides--this PR will clean up some uses of #define symbols, but it seems like we're still mostly translating them as integers.
Do you know what might help us translate LITERAL_BOOL as const LITERAL_BOOL: bool = ...;? It seems like recreate_const_macro_from_expansions might be able to realize that all uses are boolifying it.
There was a problem hiding this comment.
Do you know what might help us translate
LITERAL_BOOLasconst LITERAL_BOOL: bool = ...;? It seems likerecreate_const_macro_from_expansionsmight be able to realize that all uses are boolifying it.
I'm not sure, but I'll look into it.
00730ae to
a962605
Compare
a4708f4 to
da528f1
Compare
469e4c3 to
6c689e1
Compare
f96631d to
2e42fc9
Compare
|
One thing I'd like to include here is documentation (just a comment probably) relating the conservative/pessimistic analysis in |
fw-immunant
left a comment
There was a problem hiding this comment.
First 3 commits look good, but please add the comment/documentation I mention relating is_const_expr to ctx.is_const.
4th commit should have some test coverage before merging.
b3e290b to
a6ce89e
Compare
So you're saying the existing approach of checking |
It's currently known to be incorrect, e.g. it doesn't check function calls for constness, resulting in #803. But it's not fundamentally wrong to implement constness checking that way if we covered every possible case during translation, and for an implementation that wants to cover all cases I think it's probably the right way to implement it--we don't want a separate full AST traversal for each analysis because they would need to stay synchronized with respect to the particular way we translate every idiom so they can reason about the Rust we're generating. For now I think it's fine to have two parallel approaches (a conservative one and an aspirationally precise one) as long as we document what's going on. |
2654c30 to
a6ce89e
Compare
a6ce89e to
0f97e7d
Compare
d0c85d0 to
650e7de
Compare
38064aa to
8b6aad4
Compare
…sion_test}` into `IndexMap`s instead of `HashMap`s (#1334) * Split out of #1306. Previously, `macro_invocations` was a `HashMap`, and thus iterating through it was unordered, which populated the `Vec<CExprId>` of `macro_expansions` non-deterministically, which then resulted in non-deterministic output from `--translate-const-macros conservative`. I also changed the other `macro_*` maps to `IndexMap`, as many other maps in `TypedAstContext` are already `IndexMap`s, too, and it's likely that we want these stably ordered and deterministic.
kkysen
left a comment
There was a problem hiding this comment.
It's currently known to be incorrect, e.g. it doesn't check function calls for constness, resulting in #803. But it's not fundamentally wrong to implement constness checking that way if we covered every possible case during translation, and for an implementation that wants to cover all cases I think it's probably the right way to implement it--we don't want a separate full AST traversal for each analysis because they would need to stay synchronized with respect to the particular way we translate every idiom so they can reason about the Rust we're generating. For now I think it's fine to have two parallel approaches (a conservative one and an aspirationally precise one) as long as we document what's going on.
Fixed in 604c58a.
604c58a to
cdb7bd0
Compare
There was a problem hiding this comment.
The comments in the last commit need some fixes (see my inline comments at 604c58a), but that can be addressed in a follow up. These changes look good, but we should avoid making all translations of const macros unsafe now that we're doing them by default. The goal of the conservative const macro translation effort is to generate fully idiomatic code for cases simple enough for us to do so, so we should avoid regressing translation of the simplest cases like #define FOO 5.
So this can merge into kkysen/const-macros-unsafe-block if you like, but I'd rather not merge that branch into master while it adds unsafe to the simplest const macro translations. Merging fw/const-less-unsafe into this branch (or into kkysen/const-macros-unsafe-block after this merges into it) would be my suggested route to that.
The stacked PR workflow is confusing but hopefully the above makes sense.
kkysen
left a comment
There was a problem hiding this comment.
These changes look good, but we should avoid making all translations of const macros unsafe now that we're doing them by default. The goal of the conservative const macro translation effort is to generate fully idiomatic code for cases simple enough for us to do so, so we should avoid regressing translation of the simplest cases like
#define FOO 5.
This discussion kind of got split over multiple PRs, so I just wanted to link to the other parts here in case you miss it: #1335 (review), #1339 (review).
So this can merge into
kkysen/const-macros-unsafe-blockif you like, but I'd rather not merge that branch into master while it adds unsafe to the simplest const macro translations. Mergingfw/const-less-unsafeinto this branch (or intokkysen/const-macros-unsafe-blockafter this merges into it) would be my suggested route to that.The stacked PR workflow is confusing but hopefully the above makes sense.
I'm not sure what the point of avoiding a temporary change to master is. We're not publishing a separate version in the meantime. From what I understand, the goal is not to keep things perfect all the time, but to make steady improvements quickly. We also haven't published --translate-const-macros conservative, so I don't see how changing what that emits to be a regression. So I'd much prefer to merge these PRs in order.
…ranslate-const-macros conservative` (#1335) * Split out of #1306. Some operations, such as ptr arithmetic, are `unsafe` and can be done in const macros. So as an initially overly conservative implementation, just make all `const`s use `unsafe` blocks in case `unsafe` operations are used. This is what we already do for `fn`s, for example, even if all of the operations in a `fn` are safe. We can improve this, but for now, this works and is correct.
…c` and snapshot tests don't test `libc`
…hot (os-specific)
…re `CExprKind`s We do this by recursively checking whether an expression is `const`. This is generally done in a conservative manner, modulo a few bugs and unhandled things, namely: * the `ExplicitCast` bug (#853) * non-`const` `sizeof(VLA)`s * `sizeof`s and other `UnaryType` exprs (e.x. `alignof`) * `f128`'s non-`const` methods (#1262) * statements
cdb7bd0 to
81551c0
Compare
…re of the conservative and experimental const expr checks
81551c0 to
ecddc95
Compare
kkysen
left a comment
There was a problem hiding this comment.
The comments in the last commit need some fixes (see my inline comments at 604c58a), but that can be addressed in a follow up.
Fixed in 81551c0..ecddc95.
--translate-const-macros#803.We do this by recursively checking whether an expression is
const. This is generally done in a conservative manner, modulo theExplicitCastbug (#853), non-constsizeof(VLA)s, andf128's non-constmethods (#1262). For #853, this does generate incorrect code even onconservative, but I'll work on fixing that next.Also, because we now allow certain operations that are
unsafe, like ptr arithmetic, we wrap allconsts in anunsafeblock. This is similar to how allfns we generate are markedunsafe fns even if they don't contain anyunsafeoperations within them. We can improve this, but for now, this works and is correct.Also, the output was being non-deterministic due to the usage of
HashMaps for macro maps likemacro_invocations, so I changed these toIndexMaps that are order-preserving.