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

Consider changing or allow tweaking the field selection for synchronization #1488

Open
bbannier opened this issue Jul 18, 2023 · 1 comment
Labels
Enhancement Improvement of existing functionality

Comments

@bbannier
Copy link
Member

bbannier commented Jul 18, 2023

When parsing of a field fails we currently select the next &synchronize field after the failing field. This means that if parsing fails on the last field of the unit marked &synchronize we cannot synchronize the unit at all. For such units one would need to write custom code or need to rely on e.g., list synchronization in the outer unit (this only works if the list element unit supports lookahead parsing). Such custom code look e.g., like this:

type Message = unit {
    a: b"X";
    b: b"F"; # Field we would like to synchronize on.

    # Only used for synchronization.
    #
    # - this field is only parsed if `self.b` was left unset: `if ( ! self?.b )`
    # - it is left unnamed so it does not store data on its own and is invisible
    # - if it parses data it stores its data in the original field: `self.b = $$`
    # - for this example we confirm unconditionally
    : b"F" &synchronize if ( ! self?.b) {
        self.b = $$;
        confirm;
    }
};

This seems pretty subtle and makes me wonder whether we should change the way we pick the sync field.

  • If we started from scratch we could e.g., add the failing field the the list of fields consider and pick it if it is marked &synchronize. This would seem okay if errors causing synchronization are triggered independently of the data (gaps), but I believe could change semantics if synchronization is triggered by parser-internal code (e.g., failed &requires/overflows).
  • Alternatively we could introduce some new syntax or attribute so users need to explicitly mark such fields. The downside is that synchronization is already pretty subtle and unlikely to be fully covered with test data.
@bbannier bbannier added the Enhancement Improvement of existing functionality label Jul 18, 2023
@bbannier
Copy link
Member Author

We discussed this offline and will try to implement the first approach (i.e., also considering the current field when looking for sync candidates).

This will need at least a change to the logic finding sync candidates

// Precompute sync points for each field.
auto sync_points = std::vector<std::optional<uint64_t>>();
sync_points.reserve(p.fields().size());
for ( const auto xs : hilti::util::enumerate(p.fields()) ) {
const uint64_t field_counter = std::get<0>(xs);
bool found_sync_point = false;
for ( auto candidate_counter = field_counter + 1; candidate_counter < p.fields().size();
++candidate_counter )
if ( auto candidate = p.fields()[candidate_counter].meta().field();
candidate && AttributeSet::find(candidate->attributes(), "&synchronize") ) {
sync_points.emplace_back(candidate_counter);
found_sync_point = true;
break;
}
// If no sync point was found for this field store a None for it.
if ( ! found_sync_point )
sync_points.emplace_back();
}
as well as tests exercising different scenarios.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improvement of existing functionality
Projects
None yet
Development

No branches or pull requests

1 participant