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

Change lab01 rotation functions parameter type #171

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

c7stef
Copy link
Contributor

@c7stef c7stef commented Mar 4, 2024

Use unsigned int instead of int for bitwise rotate functions. The reason for this change is that signed integers should generally not be used as bitsets, since some arithmetic operators behave badly in the case of signed integers, as opposed to unsigned (particularly left and right shifts). Here, the problems with int arise in the following ways:

  • The proposed solution assigns the unsigned constant 0x80000000, with value 2^31, to a 32-bit signed integer, which can only represent values less than or equal to 2^31 - 1. Assigning this large value to an int has implementation-defined behavior. Legal signed integer values with this particular bit representation (1 followed by 31 zeros) include two's complement -2^31, sign-and-magnitude -0 etc. (depending on the encoding).
  • The proposed rotate_left impl shifts signed *number to the left by bits positions, though the C standard document specifies the result of this operation is *number * 2^(bits), and if this value is not representable by int, then the behavior is undefined (this happens whenever a 1 is shifted to or beyond the most significant bit).
  • The proposed rotate_right impl similarly shifts signed *number to the right by bits positions, adding the missing piece to the left by way of the | operator. The assumption is that the shift leaves some 0 bits to the left of *number. However, if the most significant bit is 1 (i.e., a negative value), the result of the right shift is implementation-defined. Modern platforms implement arithmetic right shift, which extends the sign of *number by copying the most significant bit, leaving 1 instead of 0 to the left side of *number.

All of these problems can be avoided by using unsigned numbers instead, as they do not exhibit strange behavior with bit operators. Note that using signed integers for the right-hand operands of shift operators is not a problem, hence I left the bits parameter untouched.

Copy link
Member

@teodutu teodutu left a comment

Choose a reason for hiding this comment

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

All good. And thanks for the detailed description.

We're now transitioning this content to a more open and granular standard (you can read about it here if you want [1]). In doing so, we're copying the lab content (for now; the lectures will follow) to this repo [2] and translating it into English. We intend to use this new repo starting next semester. So please apply these changes to the rotations exercise in the new repo [3] if you have the time. Remember to respect the contributing guidelines [4] as they're stricter than the ones on this repo.

[1] https://open-education-hub.github.io/methodology/
[2] https://github.com/cs-pub-ro/hardware-software-interface/
[3] https://github.com/cs-pub-ro/hardware-software-interface/tree/main/chapters/intro-computer-architecture/binary-hex/drills/tasks/rotations
[4] https://github.com/cs-pub-ro/hardware-software-interface/blob/main/CONTRIBUTING.md#commits

@teodutu teodutu merged commit 9216b71 into systems-cs-pub-ro:master Mar 5, 2024
1 check passed
@c7stef c7stef deleted the rotation-fix branch March 11, 2024 17:01
c7stef added a commit to c7stef/hardware-software-interface that referenced this pull request Mar 11, 2024
Functions of the rotation task accepted `int` parameters, which can lead
to unexpected behavior with bit operators. For more details, see
systems-cs-pub-ro/iocla#171.

Signed-off-by: Cosmin Popa <[email protected]>
teodutu pushed a commit to cs-pub-ro/hardware-software-interface that referenced this pull request Mar 19, 2024
Functions of the rotation task accepted `int` parameters, which can lead
to unexpected behavior with bit operators. For more details, see
systems-cs-pub-ro/iocla#171.

Signed-off-by: Cosmin Popa <[email protected]>
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.

2 participants