nuttx icon indicating copy to clipboard operation
nuttx copied to clipboard

userspace: Exclude nuttx/arch.h

Open Fix-Point opened this issue 1 year ago • 1 comments

Summary

The nuttx/arch.h included in userspace.h will cause some unnecessary headers to be introduced in the user program (such as net.h). We are trying to remove the inclusion of nuttx/arch.h. Also, the inclusion of nuttx/arch.h in user-space headers was causing issues because architecture-related definitions and APIs should not be exposed to users. By removing this inclusion, the patch ensures that user-space applications do not have access to or rely on architecture-specific details, which is crucial for maintaining a clean separation between user-space and kernel-space.

Impact

The impact of this change is primarily on the user-space applications that were previously using nuttx/arch.h. These applications will now need to be updated to remove the dependency on this header. The change should not affect the build process, hardware, documentation, or security.

Testing

Tested on QEMU/arm64 and QEMU/x86_64.

Fix-Point avatar Oct 16 '24 02:10 Fix-Point

[Experimental Bot, please feedback here]

The PR seems to meet most of the NuttX requirements. However, there are some areas that could be improved:

Summary:

  • Good: Provides a clear explanation of the problem (nuttx/arch.h inclusion brings unnecessary headers and exposes architecture-specific details) and the proposed solution (removing the inclusion).
  • Missing:
    • Functional part: Specify the exact files and functions modified by the change. For instance, mention that the change modifies include/nuttx/userspace.h and removes the #include <nuttx/arch.h> line.
    • How it works: While you explain the "what," elaborating on the "how" would strengthen the summary. Briefly describe the mechanism used to remove the dependency on nuttx/arch.h without breaking user-space applications.

Impact:

  • Good: Clearly states the impact on user-space applications and mentions no impact on other aspects.
  • Needs improvement:
    • User impact: Be more specific about how users need to adapt. Do they need to include a different header? Modify their code? Provide concrete examples if possible.
    • Compatibility: You mention no impact, but removing a header file can lead to compatibility issues. Clarify if this change guarantees backward compatibility. If not, specify the versions or scenarios affected.

Testing:

  • Good: Mentions the tested architectures (QEMU/arm64, QEMU/x86_64).
  • Missing:
    • Build Host: Specify the operating system and compiler used for testing.
    • Target (board:config): Mention the specific boards and configurations used for testing on each architecture.
    • Logs: The template requests "testing logs before change" and "testing logs after change." While not strictly required for every PR, providing relevant log snippets demonstrating the issue before the change and the successful outcome after the change would significantly strengthen the testing section.

In summary: The PR addresses an important issue and provides a good starting point. However, adding the missing details will make it even stronger and easier for reviewers to understand and approve.

nuttxpr avatar Oct 16 '24 03:10 nuttxpr

Sorry @Fix-Point: Could you take a look at esp32s3-devkit:eth_lan9250, is it failing because of this PR? Thanks!

Configuration/Tool: esp32s3-devkit/eth_lan9250
chip/esp32s3_efuse_lowerhalf.c: In function 'esp32s3_efuse_lowerhalf_write':
Error: chip/esp32s3_efuse_lowerhalf.c:142:11: error: implicit declaration of function 'enter_critical_section' [-Werror=implicit-function-declaration]
  142 |   flags = enter_critical_section();
      |           ^~~~~~~~~~~~~~~~~~~~~~
Error: chip/esp32s3_efuse_lowerhalf.c:152:3: error: implicit declaration of function 'leave_critical_section' [-Werror=implicit-function-declaration]
  152 |   leave_critical_section(flags);
      |   ^~~~~~~~~~~~~~~~~~~~~~

https://github.com/NuttX/nuttx/actions/runs/11630100298/job/32388417948#step:7:864

lupyuen avatar Nov 01 '24 23:11 lupyuen

Also freedom-k64f:demo and mikroe-stm32f4:fulldemo. Thanks!

https://github.com/NuttX/nuttx/actions/runs/11630100298/job/32388417242#step:7:859

Configuration/Tool: freedom-k64f/demo,CONFIG_ARM_TOOLCHAIN_GNU_EABI
Error: sensors/fxos8700cq.c:96:39: error: 'struct file' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
   96 | static int fxos8700cq_open(FAR struct file *filep);
      |                                       ^~~~

https://github.com/NuttX/nuttx/actions/runs/11630100298/job/32388420301#step:7:523

Configuration/Tool: mikroe-stm32f4/fulldemo,CONFIG_ARM_TOOLCHAIN_GNU_EABI
audio/vs1053.c: In function 'vs1053_stop':
Error: audio/vs1053.c:1579:3: error: implicit declaration of function 'up_mdelay' [-Werror=implicit-function-declaration]
 1579 |   up_mdelay(40);

lupyuen avatar Nov 01 '24 23:11 lupyuen

Also freedom-k64f:demo and mikroe-stm32f4:fulldemo. Thanks!

https://github.com/NuttX/nuttx/actions/runs/11630100298/job/32388417242#step:7:859

Configuration/Tool: freedom-k64f/demo,CONFIG_ARM_TOOLCHAIN_GNU_EABI
Error: sensors/fxos8700cq.c:96:39: error: 'struct file' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
   96 | static int fxos8700cq_open(FAR struct file *filep);
      |                                       ^~~~

https://github.com/NuttX/nuttx/actions/runs/11630100298/job/32388420301#step:7:523

Configuration/Tool: mikroe-stm32f4/fulldemo,CONFIG_ARM_TOOLCHAIN_GNU_EABI
audio/vs1053.c: In function 'vs1053_stop':
Error: audio/vs1053.c:1579:3: error: implicit declaration of function 'up_mdelay' [-Werror=implicit-function-declaration]
 1579 |   up_mdelay(40);

https://github.com/apache/nuttx/pull/14603/commits

Fix-Point avatar Nov 02 '24 08:11 Fix-Point

Sorry @Fix-Point I think we have a similar problem with esp32s3-devkit/eth_lan9250. FYI I tracked this using our new NuttX Build History Tool

Configuration/Tool: esp32s3-devkit/eth_lan9250
chip/esp32s3_efuse_lowerhalf.c: In function 'esp32s3_efuse_lowerhalf_write':
Error: chip/esp32s3_efuse_lowerhalf.c:142:11: error: implicit declaration of function 'enter_critical_section' [-Werror=implicit-function-declaration]
  142 |   flags = enter_critical_section();
      |           ^~~~~~~~~~~~~~~~~~~~~~
Error: chip/esp32s3_efuse_lowerhalf.c:152:3: error: implicit declaration of function 'leave_critical_section' [-Werror=implicit-function-declaration]
  152 |   leave_critical_section(flags);
      |   ^~~~~~~~~~~~~~~~~~~~~~

https://github.com/NuttX/nuttx/actions/runs/11630100298/job/32388417948#step:7:864

lupyuen avatar Nov 03 '24 00:11 lupyuen

Fix here: https://github.com/apache/nuttx/pull/14610, @lupyuen

xiaoxiang781216 avatar Nov 03 '24 01:11 xiaoxiang781216

this pr will lead compile error make distclean -j20; ./tools/configure.sh -l omnibusf4:nsh ;make -j20

CC: ctype/lib_tolower.c mtd/sector512.c: In function 's512_ioctl': mtd/sector512.c:554:19: error: invalid use of undefined type 'struct partition_info_s' 554 | info->numsectors = priv->neblocks * priv->stdperblock; | ^~ mtd/sector512.c:555:19: error: invalid use of undefined type 'struct partition_info_s' 555 | info->sectorsize = SECTOR_512; | ^~ mtd/sector512.c:556:19: error: invalid use of undefined type 'struct partition_info_s' 556 | info->startsector = 0; | ^~ mtd/sector512.c:557:19: error: invalid use of undefined type 'struct partition_info_s' 557 | info->parent[0] = '\0';

hujun260 avatar Nov 03 '24 02:11 hujun260

@hujun260 Fix here: https://github.com/apache/nuttx/pull/14609

xiaoxiang781216 avatar Nov 03 '24 02:11 xiaoxiang781216