Skip to content

Conversation

@cfriedt
Copy link
Member

@cfriedt cfriedt commented Dec 9, 2022

This simply adds a unit test for a previous change that went in which did not have an accompanying test.

Prior to #41602, due to the ordering of operations (first mul, then div), an intermediate value would overflow, resulting in a time non-linearity.

Fixes #41111

@cfriedt cfriedt requested a review from nashif as a code owner December 9, 2022 13:38
@cfriedt cfriedt force-pushed the test-for-z-tmcvt-overflow branch 2 times, most recently from 377d651 to 497810d Compare December 9, 2022 13:40
@cfriedt cfriedt added area: Timer Timer area: Tests Issues related to a particular existing or missing test labels Dec 9, 2022
@cfriedt cfriedt self-assigned this Dec 9, 2022
@stephanosio stephanosio assigned andyross and unassigned cfriedt Dec 12, 2022
Comment on lines 5 to 8
Copy link
Contributor

Choose a reason for hiding this comment

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

Not critical or blocking, but this is weird yaml. Does utilities.time_units.z_tmcvt: {} work?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm a YAML n00b, so my guess is it will. Are there other instances of that in-tree?

Copy link
Member Author

@cfriedt cfriedt Dec 15, 2022

Choose a reason for hiding this comment

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

@mbolivar-nordic - Fixed!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a YAML n00b, so my guess is it will.

I guess you tested it? My working tree is busy with another branch rn, which is why I phrased it as a question

Are there other instances of that in-tree?

I don't know -- did you copy/paste it from somewhere?

My example is definitely valid YAML, it just creates an empty dictionary in the test specification:

In [21]: yaml_str = '''
    ...: common:
    ...:   tags: time_units
    ...:   type: unit
    ...: tests:
    ...:   utilities.time_units.z_tmvct: {}
    ...: '''

In [22]: yaml.safe_load(yaml_str)
Out[22]: 
{'common': {'tags': 'time_units', 'type': 'unit'},
 'tests': {'utilities.time_units.z_tmvct': {}}}

from the comment on line 7 above, it looked to me like someone was looking for a way to have a dict key which mapped to an empty dict, but didn't know how to spell that in YAML.

Copy link
Member Author

@cfriedt cfriedt Dec 16, 2022

Choose a reason for hiding this comment

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

I'm a YAML n00b, so my guess is it will.

I guess you tested it?

Yep

Are there other instances of that in-tree?

I don't know -- did you copy/paste it from somewhere?

There is 1 instance of {} that I have found in-tree with grep and I only learned of it after you mentioned. But no, I did not copy-paste, but knew from my limited yaml experience that empty yaml attributes do not parse properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

but knew from my limited yaml experience that empty yaml attributes do not parse properlly.

Gotcha. The secret trick is that YAML is actually a superset of JSON, so if you don't know how to do something in YAML but you do in JSON, it works as you'd expect.

As an extreme example:

In [2]: yaml_str = '''{'common': {'tags': 'time_units', 'type': 'unit'},
   ...:  'tests': {'utilities.time_units.z_tmvct': {}}}'''

In [3]: yaml.safe_load(yaml_str)
Out[3]: 
{'common': {'tags': 'time_units', 'type': 'unit'},
 'tests': {'utilities.time_units.z_tmvct': {}}}

Prior to zephyrproject-rtos#41602, due to the ordering of operations (first mul,
then div), an intermediate value would overflow, resulting in
a time non-linearity.

This test ensures that time rolls-over properly.

Signed-off-by: Chris Friedt <[email protected]>
@fabiobaltieri fabiobaltieri merged commit 74c9c0e into zephyrproject-rtos:main Dec 19, 2022
@cfriedt cfriedt deleted the test-for-z-tmcvt-overflow branch January 24, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Tests Issues related to a particular existing or missing test area: Timer Timer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

uint64 overflow in z_tmcvt() function

5 participants