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

sys: device_mmio.h remove #include <toolchain/common.h> #41555

Merged
merged 1 commit into from
Jan 7, 2022

Conversation

marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Jan 4, 2022

v1: re-order #include <toolchain/common.h>

v2: remove #include <toolchain/common.h>

v3: replace #include <toolchain/common.h> with #include <toolchain.h>

Any version fixes the following warning:

$ west build -b qemu_x86  tests/arch/x86/static_idt/

In file included from zephyr/include/toolchain.h:50,
             from zephyr/include/linker/section_tags.h:12,
             from zephyr/include/linker/sections.h:132,
             from zephyr/include/sys/device_mmio.h:19,
             from zephyr/include/drivers/interrupt_controller/loapic.h:14,
             from zephyr/include/drivers/interrupt_controller/sysapic.h:10,
             from zephyr/include/arch/x86/arch.h:231,
             from zephyr/include/arch/cpu.h:15,
             from zephyr/tests/arch/x86/static_idt/src/test_stubs.S:17:
zephyr/include/toolchain/gcc.h:61: error: BUILD_ASSERT redefined [-Werror]
   61 | #define BUILD_ASSERT(EXPR, MSG...) _Static_assert(EXPR, "" MSG)
      |
In file included from zephyr/include/sys/device_mmio.h:18,
             from zephyr/include/drivers/interrupt_controller/loapic.h:14,
             from zephyr/include/drivers/interrupt_controller/sysapic.h:10,
             from zephyr/include/arch/x86/arch.h:231,
             from zephyr/include/arch/cpu.h:15,
             from zephyr/tests/arch/x86/static_idt/src/test_stubs.S:17:
zephyr/include/toolchain/common.h:165: note: this is the location of the
  previous definition
  165 | #define BUILD_ASSERT(EXPR, MSG...) \

Thanks to Gerard Marull-Paretas for recommending v3

Related to commit af20208 ("devices: mark device MMIO declarations to
boot/pinned sections") that added #include <linker/sections.h>

#include <toolchain/common.h> was here since the dawn of time (= commit db0ca08)

Signed-off-by: Marc Herbert marc.herbert@intel.com

@github-actions github-actions bot added the area: API Changes to public APIs label Jan 4, 2022
@marc-hb
Copy link
Collaborator Author

marc-hb commented Jan 4, 2022

Warning found in unrelated https://github.com/zephyrproject-rtos/zephyr/runs/4697416054 / #41553

@marc-hb marc-hb changed the title sys: device_mmio.h #include linker/sections.h after toolchain/common.h sys: device_mmio.h #include linker/sections.h before toolchain/common.h Jan 4, 2022
@marc-hb marc-hb marked this pull request as ready for review January 4, 2022 05:15
marc-hb referenced this pull request Jan 4, 2022
This adds to the macros for device MMIO declaration so they can
be put into  boot or pinned linker sections as needed.

Signed-off-by: Daniel Leung <daniel.leung@intel.com>
@marc-hb marc-hb requested a review from gmarull January 4, 2022 05:20
@marc-hb marc-hb added area: Portability Standard compliant code, toolchain abstraction area: Toolchains Toolchains labels Jan 4, 2022
Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

IMO, this change doesn't really resolve the original problem. First, <sys/device_mmio.h> seems to be a header that is not self-contained (e.g., relies on DT API but doesn't include <devicetree.h>). Second, is <toolchain/common.h> really needed? Removing it seems to resolve the issue, too. Related issue: #41543

@zephyrbot zephyrbot added the area: Base OS Base OS Library (lib/os) label Jan 4, 2022
@marc-hb marc-hb changed the title sys: device_mmio.h #include linker/sections.h before toolchain/common.h sys: device_mmio.h remove #include <toolchain/common.h> Jan 4, 2022
@@ -15,7 +15,6 @@
#ifndef ZEPHYR_INCLUDE_SYS_DEVICE_MMIO_H
#define ZEPHYR_INCLUDE_SYS_DEVICE_MMIO_H

#include <toolchain/common.h>
Copy link
Member

Choose a reason for hiding this comment

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

The file seems to make use of ARG_UNUSED, <toolchain.h> is probably the right header then?

@marc-hb
Copy link
Collaborator Author

marc-hb commented Jan 4, 2022

As mentioned in the commit message, this #include <linker/sections.h> was added by commit af20208 for __pinned_rodata and __pinned_bss (not for ARG_UNUSED which sounds like another, different problem). These __pinned_* are defined in include/linker/section_tags.h which is part of include/linker/sections.h which is part of the "everything bagel" kernel_includes.h which is why they're always found.

In this particular test tests/arch/x86/static_idt/, sys/device_mmio.h is never included directly. It's always included through kernel_includes.h:

In [sys/device_mmio.h] file included
                 from zephyr/include/drivers/interrupt_controller/loapic.h:14,
                 from zephyr/include/drivers/interrupt_controller/sysapic.h:10,
                 from zephyr/include/arch/x86/arch.h:231,
                 from zephyr/include/arch/cpu.h:15,
                 from zephyr/include/kernel_includes.h:33

Related issue: #41543

Yes that's why I copied you :-)

In include/sys/device_mmio.h, replacing <toolchain/common.h>
fixes the following warning:

$ west build -b qemu_x86  tests/arch/x86/static_idt/

In file included from zephyr/include/toolchain.h:50,
             from zephyr/include/linker/section_tags.h:12,
             from zephyr/include/linker/sections.h:132,
             from zephyr/include/sys/device_mmio.h:19,
             from zephyr/include/drivers/interrupt_controller/loapic.h:14,
             from zephyr/include/drivers/interrupt_controller/sysapic.h:10,
             from zephyr/include/arch/x86/arch.h:231,
             from zephyr/include/arch/cpu.h:15,
             from zephyr/tests/arch/x86/static_idt/src/test_stubs.S:17:
zephyr/include/toolchain/gcc.h:61: error: BUILD_ASSERT redefined [-Werror]
   61 | #define BUILD_ASSERT(EXPR, MSG...) _Static_assert(EXPR, "" MSG)
      |
In file included from zephyr/include/sys/device_mmio.h:18,
             from zephyr/include/drivers/interrupt_controller/loapic.h:14,
             from zephyr/include/drivers/interrupt_controller/sysapic.h:10,
             from zephyr/include/arch/x86/arch.h:231,
             from zephyr/include/arch/cpu.h:15,
             from zephyr/tests/arch/x86/static_idt/src/test_stubs.S:17:
zephyr/include/toolchain/common.h:165: note: this is the location of the
  previous definition
  165 | #define BUILD_ASSERT(EXPR, MSG...) \

<toolchain.h> provides a compiler-specific BUILD_ASSERT.
<toolchain/common.h> provides a generic, fallback BUILD_ASSERT and
should probably never be included directly.

Thanks to Gerard Marull-Paretas for recommending this fix.

Related to commit af20208 ("devices: mark device MMIO declarations
to boot/pinned sections") that added #include <linker/sections.h>

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
Copy link
Collaborator Author

marc-hb commented Jan 5, 2022

Slightly different and interesting warning reproduction when removing #include <linker/sections.h> and keeping #include <toolchain/common.h>

Just another datapoint / faint glow in the big and scary #include darkness #41543

$ west build -b qemu_x86 -d b0 tests/arch/x86/static_idt/

[5/100] Building ASM object CMakeFiles/app.dir/src/test_stubs.S.obj
In file included from zephyr/include/toolchain.h:50,
                 from zephyr/include/arch/x86/ia32/asm.h:12,
                 from zephyr/tests/arch/x86/static_idt/src/test_stubs.S:19:
zephyr/include/toolchain/gcc.h:61: warning: "BUILD_ASSERT" redefined
   61 | #define BUILD_ASSERT(EXPR, MSG...) _Static_assert(EXPR, "" MSG)
      | 
In file included from zephyr/include/sys/device_mmio.h:18,
                 from zephyr/include/drivers/interrupt_controller/loapic.h:14,
                 from zephyr/include/drivers/interrupt_controller/sysapic.h:10,
                 from zephyr/include/arch/x86/arch.h:231,
                 from zephyr/include/arch/cpu.h:15,
                 from zephyr/tests/arch/x86/static_idt/src/test_stubs.S:17:
zephyr/include/toolchain/common.h:165: note: this is the location of the previous definition
  165 | #define BUILD_ASSERT(EXPR, MSG...) \
      | 

@marc-hb marc-hb requested a review from tejlmand January 5, 2022 03:12
@gmarull gmarull added the bug The issue is a bug, or the PR is fixing a bug label Jan 5, 2022
@nashif nashif merged commit 61fe69d into zephyrproject-rtos:main Jan 7, 2022
@marc-hb marc-hb deleted the mmio-include-order branch January 7, 2022 19:00
@marc-hb
Copy link
Collaborator Author

marc-hb commented Jan 11, 2022

Twister fix that would have caught this earlier: twister: add -Werror to CMAKE_AFLAGS #41706

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Base OS Base OS Library (lib/os) area: Portability Standard compliant code, toolchain abstraction area: Toolchains Toolchains bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants