Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep original label names from C code #415

Merged
merged 2 commits into from May 23, 2022

Conversation

afetisov
Copy link
Contributor

I have added an Option<Rc<str>> field to the cfg::Label::FromC. This allows to preserve the names of blocks which are provided in the C source, potentially increasing the readability of transpiled code and making it easier to match original and transpiled code blocks. The label names are used as control flow variable values when --ddebug-labels is enabled. Unfortunately, Relooper really likes to dump those names in the _ branch of matches, bringing the automatic label names to the front. Still, there are some situations where the old labels can be traced in the transpiled code. This will also significantly increase readability if Relooper is changed to use labeled blocks & breaks instead of a match on variable: the C label names can be directly used as block label names.

I had to make Label non-Copy. This is not a painful transition, and arguably there was no reason to make it Copy in the first place, since it was likely to carry some non-Copy data.

This allows to put the label name into the label structure, and doesn't significantly affect the rest of the code.
This allows to keep the names of explicit labels defined in the C source. Note that not all labels
coming from C have a user-defined name, e.g. case statements within a switch statements are anonymous.
Copy link
Contributor

@fw-immunant fw-immunant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good overall. My first thought was that it would be nice to keep Label: Copy, but that would mean accessing label_names in Label::pretty_print, and most callers of the latter don't have access to the TypedAstContext. So this approach seems sane and the implementation looks reasonable. The use of Rc here does seem worthwhile given how many .clone()s we end up introducing.

Most of the panics introduced seem to be only in places where we're relying on internal consistency of label_names with the set of AST locations where we expect names, so they shouldn't hurt overally reliability. The one exception is:

                    let label_name = from_value::<Rc<str>>(node.extras[0].clone())
                        .expect("unnamed label in C source code");

where we're relying on the serialized C AST having the structure we expect. This is the same pattern as all the adjacent code, so I'm satisfied with tests passing to validate it.

As far as improving output of translation goes, there are a few improvements in the output for our test cases, e.g. gotos/irreducible.c where this change now brings over the l1 label. 👍

@fw-immunant fw-immunant merged commit 4f956d1 into immunant:master May 23, 2022
@afetisov afetisov deleted the c-label-names branch July 14, 2022 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants