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

Add missing parentheses to some important macros #4775

Merged
merged 2 commits into from
Jan 8, 2019

Conversation

vomindoraan
Copy link
Contributor

@vomindoraan vomindoraan commented Jan 4, 2019

Add missing parentheses around arguments in macros like LT, MO, TT, pgm_read_byte etc.

Description

This is done to prevent unintended grouping of operands due to operator precedence. Take the MO macro in its current state, for example:

#define MO(layer) (QK_MOMENTARY | (layer & 0xFF))

Calling this macro with e.g. MO(cond ? _RAISE : _LOWER) would result in unexpected behavior. The macro would be expanded like so: (QK_MOMENTARY | (cond ? _RAISE : _LOWER & 0xFF)). Then, because of operator precedence, _LOWER & 0xFF would be grouped together, which likely isn't what was intended.
To fix this, we instead define MO as follows:

#define MO(layer) (QK_MOMENTARY | ((layer) & 0xFF))

Same goes for

#define pgm_read_dword(p) *((uint32_t*)p)

If we call that with pgm_read_dword(unicode_map + index), unicode_map will be cast to a pointer before addition, likely resulting in the wrong address being calculated due to how pointer arithmetic works.
The argument, p, must be wrapped in parentheses:

#define pgm_read_dword(p) *((uint32_t*)(p))

This is a common pitfall in C. Macro arguments should always be parenthesized. See this SO question for more information.

Types of changes

  • Core
  • Bugfix
  • New Feature
  • Enhancement/Optimization
  • Keyboard (addition or update)
  • Keymap/Layout/Userspace (addition or update)
  • Documentation

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document. (https://docs.qmk.fm/#/contributing)
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@vomindoraan vomindoraan changed the title Add missing parentheses to quantum_keycodes macros Add missing parentheses to some important macros Jan 4, 2019
@drashna
Copy link
Member

drashna commented Jan 6, 2019

Tested AVR and ARM real quickly, and doesn't appear to be any impact.

Can't test ATSAM unless somebody wants to ship a massdrop board to me for testing

@vomindoraan
Copy link
Contributor Author

vomindoraan commented Jan 6, 2019

After a bit of searching through the codebase, I think these are the only potentially problematic lines:

https://dl.dropboxusercontent.com/s/sp4g2hw4fwigilq/Screenshot%202019-01-06%2020.00.35%20%28bash%29.png

As text drivers/avr/analog.c:42
drivers/avr/pro_micro.h:134
drivers/avr/ssd1306.c:288
drivers/qwiic/micro_oled.c:615
drivers/qwiic/micro_oled.c:616
drivers/qwiic/micro_oled.c:617
drivers/qwiic/micro_oled.c:618
drivers/qwiic/micro_oled.c:619
drivers/qwiic/micro_oled.c:636
drivers/qwiic/micro_oled.c:662
drivers/qwiic/micro_oled.c:683
keyboards/comet46/lib/keylogger.c:282
keyboards/comet46/ssd1306.c:300
keyboards/crkbd/pro_micro.h:132
keyboards/crkbd/ssd1306.c:300
keyboards/helix/pro_micro.h:134
keyboards/helix/ssd1306.c:298
keyboards/lily58/ssd1306.c:300
keyboards/minidox/pro_micro.h:134
keyboards/sol/common/ssd1306.c:292
keyboards/zeal60/rgb_backlight.c:296
keyboards/zeal60/rgb_backlight.c:344
quantum/process_keycode/process_steno.c:138
tmk_core/protocol/usb_hid/arduino-1.0.1/cores/arduino/Arduino.h:141
tmk_core/protocol/usb_hid/arduino-1.0.1/cores/arduino/Arduino.h:142
tmk_core/protocol/usb_hid/arduino-1.0.1/cores/arduino/Arduino.h:143
tmk_core/protocol/usb_hid/arduino-1.0.1/cores/arduino/Arduino.h:145
tmk_core/protocol/usb_hid/arduino-1.0.1/cores/arduino/Arduino.h:146
tmk_core/protocol/usb_hid/arduino-1.0.1/cores/arduino/Arduino.h:147
tmk_core/protocol/usb_hid/arduino-1.0.1/variants/leonardo/pins_arduino.h:67
tmk_core/protocol/vusb/usbdrv/usbportability.h:129

@vomindoraan
Copy link
Contributor Author

vomindoraan commented Jan 6, 2019

However, these lines are only problematic if the p in pgm_read_byte(p + i) isn't a pointer to a byte (i.e. uint8_t *); for example, if it's a uint16_t *. The same applies to the other two macros: pgm_read_word is broken if the pointer isn't a uint16_t *, and pgm_read_dword is broken if the pointer isn't a uint32_t *.

If the pointer type matches, then it will work with both the parenthesized and non-parenthesized version, since the size doesn't change and thus the pointer arithmetic stays the same. I'll go through the above lines and see which ones have pointers that don't match and would therefore break with the proposed changes.

@yiancar
Copy link
Contributor

yiancar commented Jan 8, 2019

Zeal60 part works

@vomindoraan
Copy link
Contributor Author

I've checked all of the potentially problematic lines listed above, and the types match in each of them! So these changes shouldn't introduce any problems, and this PR should be good to go.

@drashna drashna merged commit 64c957d into qmk:master Jan 8, 2019
@vomindoraan vomindoraan deleted the macro_parentheses branch January 9, 2019 00:57
Shinichi-Ohki added a commit to Shinichi-Ohki/qmk_firmware that referenced this pull request Jan 10, 2019
* 'master' of https://github.com/qmk/qmk_firmware: (63 commits)
  Keymap: ave-63's iris layout (qmk#4812)
  Update bakingpy 4x12 keymap and add test mode for debugging/development (qmk#4810)
  Fix Mac mute keycode (qmk#4809)
  Add KBD75 keymap (qmk#4808)
  Fix pinout of split hand and LED, remove flip half option
  Tidy up Mod-Tap shortcuts (qmk#4806)
  Add missing parentheses to some important macros (qmk#4775)
  Keyboard: Downbubble refactor and Configurator fix (qmk#4798)
  Alternate keymap for Alpha keyboard, enjoy! (qmk#4797)
  Keymap: Added Model F-inspired layout for YMD[K]96 (qmk#4777)
  Improve consistency in UNICODEMAP code and docs, update docs/understanding_qmk (qmk#4774)
  Update to arm_atsam wait and timer routines
  Add Downbubble to Handwired repository (qmk#4794)
  Final HS60v2 changes. (qmk#4790)
  Keyboard: Fractal layout macro and readme cleanup (qmk#4789)
  Keymap: added my espectro keymap (qmk#4791)
  Keyboard: Numbrero: Configurator fix and code tidy (qmk#4787)
  Keyboard: Tradestation code tidy and readme refactor (qmk#4784)
  Keyboard: update readme with ps2avr flashing instructions (qmk#4776)
  add Pinky keyboard (qmk#4748)
  ...
rseymour pushed a commit to rseymour/qmk_firmware that referenced this pull request Mar 13, 2019
* Add missing parentheses to quantum_keycodes macros

* Add missing parentheses to progmem macros
djthread pushed a commit to djthread/qmk_firmware that referenced this pull request Mar 17, 2019
* Add missing parentheses to quantum_keycodes macros

* Add missing parentheses to progmem macros
dlhextall pushed a commit to dlhextall/qmk_firmware that referenced this pull request May 24, 2019
* Add missing parentheses to quantum_keycodes macros

* Add missing parentheses to progmem macros
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants