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

CMakeLists.txt: -fmacro-prefix-map=${CMAKE_SOURCE_DIR}=CMAKE_SOURCE_DIR #16536

Merged
merged 1 commit into from
Jun 17, 2019

Conversation

marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Jun 1, 2019

In commit 28a5657 we stopped ZEPHYR_BASE from leaking into
__FILE__ and other macros. This works great for apps inside ZEPHYR_BASE
but does nothing for apps outside ZEPHYR_BASE.
-fmacro-prefix-map=${CMAKE_SOURCE_DIR}=CMAKE_SOURCE_DIR does it.

To avoid collisions and for consistency change:
-fmacro-prefix-map=${ZEPHYR_BASE}=.
to:
-fmacro-prefix-map=${ZEPHYR_BASE}=ZEPHYR_BASE

Quickest test/demo:

  • Copy samples/hello_word/ to ~/home_world/
  • Add "%s", __FILE__ to printf in ~/home_world/src/main.c

cmake -B build -S ~/home_world/ -DBOARD=qemu_x86
make -C build run

Before:
~/home_world/src/main.c says Hello World! qemu_x86

After:
CMAKE_SOURCE_DIR/src/main.c says Hello World! qemu_x86

objdump -h $(find build -name *.c.obj) | grep noinit

9 .noinit."ZEPHYR_BASE/kernel/system_work_q.c".0 0000
17 .noinit."ZEPHYR_BASE/kernel/init.c".2 0000010 000
18 .noinit."ZEPHYR_BASE/kernel/init.c".1 00000400 000
19 .noinit."ZEPHYR_BASE/kernel/init.c".3 00000800 000
16 .noinit."ZEPHYR_BASE/kernel/mailbox.c".2 00000348
18 .noinit."ZEPHYR_BASE/kernel/mailbox.c".1 00000028
20 .noinit."ZEPHYR_BASE/kernel/pipes.c".2 00000280 00
22 .noinit."ZEPHYR_BASE/kernel/pipes.c".1 00000028 00

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

Copy link
Collaborator

@mped-oticon mped-oticon left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@ozhuraki ozhuraki left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@SebastianBoe SebastianBoe left a comment

Choose a reason for hiding this comment

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

Can this create aliasing issues between ZEPHYR_BASE and the app dir?

Could -fmacro-prefix-map=${CMAKE_SOURCE_DIR}=app resolve this?

@marc-hb
Copy link
Collaborator Author

marc-hb commented Jun 3, 2019

Can this create aliasing issues between ZEPHYR_BASE and the app dir?

Can you be more specific?

Could -fmacro-prefix-map=${CMAKE_SOURCE_DIR}=app resolve this?

Why not.

@SebastianBoe
Copy link
Collaborator

SebastianBoe commented Jun 3, 2019

Can this create aliasing issues between ZEPHYR_BASE and the app dir?

Can you be more specific?

For instance, Zephyr has the file zephyr/include/init.h. Imagine that a user has a different file located at app_dir/include/init.h. Then after pre-processing, the path would look like ./include/init.h. Now imagine that, for whatever reason, some tooling needs to be able to deduce whether this is referencing the zephyr or app init.h. It would not be able to, a human would also be confused, but should at least eventually be able to figure it out.

EDIT: If we introduce =app, I suppose we should introduce =zephyr as well to be consistent.

@marc-hb marc-hb requested a review from mbolivar June 3, 2019 20:08
@marc-hb
Copy link
Collaborator Author

marc-hb commented Jun 3, 2019

Thanks @SebastianBoe, makes sense.

I'll just wait a couple days to give people a chance to bikeshed the prefix names app/ and zephyr/.

Shooting first: zephyr_base/?

@mbolivar
Copy link
Contributor

mbolivar commented Jun 3, 2019

Shooting first: zephyr_base/?

ZEPHYR_BASE?

@SebastianBoe
Copy link
Collaborator

If we go for ZEPHYR_BASE then CMAKE_HOME_DIRECTORY would be the corresponding name for the app.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Jun 4, 2019

I think you meant CMAKE_SOURCE_DIR?

https://cmake.org/cmake/help/latest/variable/CMAKE_HOME_DIRECTORY.html

CMAKE_HOME_DIR should not be used in project code. The variable CMAKE_SOURCE_DIR has the same value and should be preferred.

Not like this would be "using it in project code" for real but it would still be a bad example (I had never heard of CMAKE_HOME_DIR until now)

@SebastianBoe
Copy link
Collaborator

Right, CMAKE_SOURCE_DIR would be better.

@mbolivar
Copy link
Contributor

mbolivar commented Jun 4, 2019

Random question, but what about zephyr modules? That doesn't seem to be covered in this PR (no problem, incremental progress is good!), but I'm curious what ought to be done while we are thinking about this.

ZEPHYR_MODULE/<mumble-mumble> ?

@marc-hb
Copy link
Collaborator Author

marc-hb commented Jun 4, 2019

Random question, but what about zephyr modules?

Very good and not so random question. I haven't observed any module actually triggering any difference for me yet but I guess a list of -fmacro-prefix-map= options could be dynamically generated from build/zephyr_modules.txt ?

Note this is (at least) the second issue caused by moving modules higher than ZEPHYR_BASE, here's another, cmake one I've been working on: https://cmake.org/pipermail/cmake/2019-June/069547.html

Don't get me wrong: nesting git repositories is horrible so I'm not even thinking about that.

Maybe a new WEST_TOP gradually replacing ZEPHYR_BASE? I think it's been mentioned in unrelated context(s). Just thinking out loud.

In commit 28a5657 we stopped ZEPHYR_BASE from leaking into
__FILE__ and other macros.  This works great for apps inside ZEPHYR_BASE
but does nothing for apps outside ZEPHYR_BASE.
-fmacro-prefix-map=${CMAKE_SOURCE_DIR}=CMAKE_SOURCE_DIR does it.

To avoid collisions and for consistency change:
  -fmacro-prefix-map=${ZEPHYR_BASE}=.
to:
  -fmacro-prefix-map=${ZEPHYR_BASE}=ZEPHYR_BASE

Quickest test/demo:

 - Copy samples/hello_word/ to ~/home_world/
 - Add "%s", __FILE__  to printf in ~/home_world/src/main.c

 cmake -B build  -S ~/home_world/  -DBOARD=qemu_x86
 make  -C build  run

Before:
     ~/home_world/src/main.c says Hello World! qemu_x86

After:
 CMAKE_SOURCE_DIR/src/main.c says Hello World! qemu_x86

objdump -h $(find build -name *.c.obj) | grep noinit

  9 .noinit."ZEPHYR_BASE/kernel/system_work_q.c".0 0000
 17 .noinit."ZEPHYR_BASE/kernel/init.c".2 0000010  000
 18 .noinit."ZEPHYR_BASE/kernel/init.c".1 00000400  000
 19 .noinit."ZEPHYR_BASE/kernel/init.c".3 00000800  000
 16 .noinit."ZEPHYR_BASE/kernel/mailbox.c".2 00000348
 18 .noinit."ZEPHYR_BASE/kernel/mailbox.c".1 00000028
 20 .noinit."ZEPHYR_BASE/kernel/pipes.c".2 00000280  00
 22 .noinit."ZEPHYR_BASE/kernel/pipes.c".1 00000028  00

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@mbolivar
Copy link
Contributor

mbolivar commented Jun 4, 2019

Maybe a new WEST_TOP gradually replacing ZEPHYR_BASE? I think it's been mentioned in unrelated context(s). Just thinking out loud.

I want to reach for that too, but we need a solution that works without assuming west, for those easterners out there ;).

@marc-hb marc-hb changed the title CMakeLists.txt: add -fmacro-prefix-map=${CMAKE_SOURCE_DIR}=. CMakeLists.txt: -fmacro-prefix-map=${CMAKE_SOURCE_DIR}=CMAKE_SOURCE_DIR Jun 4, 2019
@marc-hb marc-hb requested a review from SebastianBoe June 4, 2019 22:41
@marc-hb
Copy link
Collaborator Author

marc-hb commented Jun 5, 2019

https://app.shippable.com/github/zephyrproject-rtos/zephyr/runs/43105/5/console 2h time out, stuck at

JOBS: 12
Selecting default platforms per test case
Building testcase defconfigs...

4 other Shippable shards PASS

@marc-hb marc-hb closed this Jun 5, 2019
@marc-hb marc-hb reopened this Jun 5, 2019
@marc-hb
Copy link
Collaborator Author

marc-hb commented Jun 11, 2019

I haven't observed any module actually triggering any difference for me yet

Famous last words.

sanitycheck -T $ZEPHYR_BASE/tests/posix/fs/ -p qemu_x86
readelf --wide --decompress --string-dump=.rodata.disk_status.str1.1 sanity-out/qemu_x86/tests/posix/fs/portability.posix/modules/fatfs/CMakeFiles/..__modules__fs__fatfs.dir/zfs_diskio.c.obj
  [     0]  /home/bob/tmp/repro/westisbest/modules/fs/fatfs/zfs_diskio.c
  [    4c]  pdrv < ((long) (((int) sizeof(char[1 - 2 * !(!__builtin_types_compatible_p(__typeof__(pdrv_str), __typeof__(&(pdrv_str)[0])))]) - 1) + (sizeof(pdrv_str) / sizeof((pdrv_str)[0]))))
  [   100]  ASSERTION FAIL [%s] @ %s:%d^J
  [   11e]  pdrv out-of-range^J^J

Any ASSERT in a module should reproduce

Quick and dirty hack below, moving on for now... bigger fish to fry.

get_filename_component(west_top ${ZEPHYR_BASE} DIRECTORY)
zephyr_cc_option(-fmacro-prefix-map=${west_top}=WEST_TOP)

@marc-hb marc-hb added area: Toolchains Toolchains Enhancement Changes/Updates/Additions to existing features labels Jun 15, 2019
@marc-hb
Copy link
Collaborator Author

marc-hb commented Jun 17, 2019

Non-urgent ping? 2 lines, 5 approvers :-)

@nashif nashif merged commit f67dcdb into zephyrproject-rtos:master Jun 17, 2019
@marc-hb marc-hb deleted the macro-prefix-source-dir branch June 25, 2019 18:53
@marc-hb
Copy link
Collaborator Author

marc-hb commented Sep 26, 2019

Maybe a new WEST_TOP gradually replacing ZEPHYR_BASE? I think it's been mentioned in unrelated context(s). Just thinking out loud.

TA-DA! zephyrproject-rtos/west#311

@marc-hb
Copy link
Collaborator Author

marc-hb commented Sep 27, 2019

cmake: -fmacro-prefix-map=${WEST_TOPDIR}=WEST_TOPDIR #19440

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System area: Toolchains Toolchains Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants