arch: avoid mixing types #73077
arch: avoid mixing types #73077
Conversation
zephyrbot
added
area: X86
zephyrbot
requested review from
andyross,
bbolen,
carlocaione,
ceolin,
dcpleung,
edersondisouza,
galak,
ithinuel,
laurenmurphyx64,
MaureenHelm,
microbuilder,
najumon1980,
nashif and
stephanosio
DeHess
force-pushed
the
misra_rule_accurate_datatypes_auditable_to_main_arch
branch
from
ef7799b
to
3855e5e
Compare
arch/arm/core/cortex_m/irq_manage.c
Outdated
| @@ -88,7 +88,7 @@ void z_arm_irq_priority_set(unsigned int irq, unsigned int prio, uint32_t flags) | |||
| __ASSERT(prio <= (BIT(NUM_IRQ_PRIO_BITS) - 1), | |||
| "invalid priority %d for %d irq! values must be less than %lu\n", | |||
| prio - _IRQ_PRIO_OFFSET, irq, | |||
| BIT(NUM_IRQ_PRIO_BITS) - (_IRQ_PRIO_OFFSET)); | |||
| (unsigned long)BIT(NUM_IRQ_PRIO_BITS) - (_IRQ_PRIO_OFFSET)); | |||
There was a problem hiding this comment.
Pretty sure the bug here is in the printf string. The argument should be "%u" and not "%lu", the NVIC priorities are limited to a single word mask no?
There was a problem hiding this comment.
Hm... there's a lot of arguable stuff in this PR, and the arch layers are subtle and error-prone, with a limited population of people who can puzzle out new interactions.
Basically this just creeps me out a bit. Can you split some of these up thematically into smaller chunks, and/or maybe provide the list of squawks as a bug to the Intel folks and let them handle the actual patching?
|
|
||
| static bool early_serial_init_done; | ||
| static uint32_t suppressed_chars; | ||
|
|
||
| static void serout(int c) | ||
| static void serout(uint8_t c) |
There was a problem hiding this comment.
Not understanding the API change here? It's not incorrect, but I don't see how promoting the character values to int is wrong either.
| @@ -52,7 +52,7 @@ bool z_x86_check_stack_bounds(uintptr_t addr, size_t size, uint16_t cs) | |||
| if (_current == NULL || arch_is_in_isr()) { | |||
| /* We were servicing an interrupt or in early boot environment | |||
| * and are supposed to be on the interrupt stack */ | |||
| int cpu_id; | |||
| uint8_t cpu_id; | |||
There was a problem hiding this comment.
Again confused. It's true that the "id" field of struct _cpu is a byte, but that's an implementation detail. Code working with it (which is rare, and limited to low level drivers like this) wants to be using a machine word for portability and simplicity.
| @@ -471,29 +469,29 @@ void z_x86_page_fault_handler(z_arch_esf_t *esf) | |||
| dump_page_fault(esf); | |||
| #endif | |||
| #ifdef CONFIG_THREAD_STACK_INFO | |||
| if (z_x86_check_stack_bounds(esf_get_sp(esf), 0, esf->cs)) { | |||
| z_x86_fatal_error(K_ERR_STACK_CHK_FAIL, esf); | |||
| if (z_x86_check_stack_bounds(esf_get_sp(esf), 0, (uint16_t)esf->cs)) { | |||
There was a problem hiding this comment.
Would strongly prefer to change the type in that struct (CS is a 16 bit register) or in that function signature (to take an int instead of a short) vs. adding a cast.
| if (z_x86_check_stack_bounds(esf_get_sp(esf), 0, esf->cs)) { | ||
| z_x86_fatal_error(K_ERR_STACK_CHK_FAIL, esf); | ||
| if (z_x86_check_stack_bounds(esf_get_sp(esf), 0, (uint16_t)esf->cs)) { | ||
| z_x86_fatal_error((unsigned int)K_ERR_STACK_CHK_FAIL, esf); |
There was a problem hiding this comment.
What's up here? If the defined error enumerants don't work to pass to the fatal error handler, we have an API bug. A cast is the wrong solution.
arch/x86/core/early_serial.c
Outdated
| @@ -81,10 +81,10 @@ int arch_printk_char_out(int c) | |||
| return c; | |||
| } | |||
|
|
|||
| if (c == '\n') { | |||
| serout('\r'); | |||
| if (c == (int)'\n') { | |||
There was a problem hiding this comment.
All but certain character constants have integer type already, no? This case is a noop per the language spec.
DeHess
force-pushed
the
misra_rule_accurate_datatypes_auditable_to_main_arch
branch
5 times, most recently
from
91b238f
to
71c1f09
Compare
DeHess
force-pushed
the
misra_rule_accurate_datatypes_auditable_to_main_arch
branch
2 times, most recently
from
5c1a027
to
5a92724
Compare
- avoid mixing signed and unsigned integers in computations and comparisons; - avoid mixing characters with integers; - added U suffix to unsigned constants (except for the CONFIG_* macro constants, as they cannot be changed and then their use as unsigned constants should be prefixed with a cast). Signed-off-by: Hess Nathan <nhess@baumer.com>
replaced manual Array size calculation with ARRAY_SIZE macro Signed-off-by: Hess Nathan <nhess@baumer.com>
DeHess
force-pushed
the
misra_rule_accurate_datatypes_auditable_to_main_arch
branch
from
5a92724
to
c184993
Compare
|
@DeHess please take a look at the twister failures. this also needs a rebase due to merge conflicts |
Fixed some occurrences of mixing up types in arch
This PR is part of the enhancement issue #48002 which port the coding guideline fixes done by BUGSENG on the https://github.com/zephyrproject-rtos/zephyr/tree/v2.7-auditable-branch back to main
The commit in this PR is a subset of the original auditable-branch commit:
d03fa8d