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

x86-64: Support symbol table and kasan global variables cross-border detection #14885

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

W-M-R
Copy link
Contributor

@W-M-R W-M-R commented Nov 21, 2024

Summary

  1. Add kasan compilation options
  2. Modify the link process to support symbol tables and kasan global variables cross-border detection

Impact

Update this section, where applicable, on how change affects users,
build process, hardware, documentation, security, compatibility, etc.

Testing

Update this section with details on how did you verify the change,
what Host was used for build (OS, CPU, compiler, ..), what Target was
used for verification (arch, board:config, ..), etc. Providing build
and runtime logs from before and after change is highly appreciated.

@github-actions github-actions bot added Arch: x86 Issues related to the x86 architecture Size: S The size of the change in this PR is small labels Nov 21, 2024
@W-M-R W-M-R force-pushed the x86-64 branch 2 times, most recently from 2886e35 to 7a1268a Compare November 21, 2024 02:59
@W-M-R W-M-R changed the title x86-64: Support symbol table and kasan global variables x86-64: Support symbol table and kasan global variables cross-border detection Nov 21, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 21, 2024

[Experimental Bot, please feedback here]

This PR does not fully meet the NuttX requirements as presented. While it provides a summary of the changes, it lacks crucial details in the Impact and Testing sections.

Here's a breakdown:

Summary: While it mentions the what, it lacks the why and how. It needs to explain the rationale behind adding KASAN support and the specific modifications made to the link process. For example, why is KASAN support being added? Is it to address a specific memory issue or for general robustness? How does modifying the link process enable cross-border detection? What specific changes were made? A related issue number would also be helpful if one exists.

Impact: This section is entirely incomplete. All points need to be addressed. KASAN will almost certainly have an impact on build size and potentially performance. It might also affect compatibility with existing code. These potential impacts need to be documented. Does enabling KASAN require any specific configuration options? Will users need to adapt their code? Does it introduce any security considerations? These questions must be answered.

Testing: This section is also incomplete. It needs to specify the host operating system, CPU architecture, compiler version, and target architecture and board configuration used for testing. Critically, it needs to include actual testing logs from before and after the change, demonstrating the effectiveness of the KASAN implementation. Simply stating that changes were verified is insufficient. Concrete evidence is required.

In short, the PR needs to be significantly more detailed to meet the NuttX requirements. It needs to provide a comprehensive explanation of the changes, their impact, and thorough testing results.

…detection

1. Add kasan compilation options
2. Modify the link process to support symbol tables and kasan global variables cross-border detection

Signed-off-by: wangmingrong1 <wangmingrong1@xiaomi.com>
@W-M-R
Copy link
Contributor Author

W-M-R commented Nov 21, 2024

Why does my patch cause issues with other architectures 🧐, It shouldn't be

@xiaoxiang781216
Copy link
Contributor

Why does my patch cause issues with other architectures 🧐, It shouldn't be

it's a known issue, @lupyuen is looking at.

@xiaoxiang781216 xiaoxiang781216 merged commit 6cd4377 into apache:master Nov 21, 2024
38 of 39 checks passed
@lupyuen
Copy link
Member

lupyuen commented Nov 21, 2024

Thanks @W-M-R: We just discovered a new bug in the Build Rules!

  • This PR is labelled "Arch: x86". Which is a Simple PR.

  • Expected Behaviour is to run the CI Build for One Single Target Group: other(x86). The CI Build for other should complete within 1 hour.

  • Actual Behaviour: It runs the CI Build for All Target Groups! (Arm, RISC-V, Xtensa, ...) Which runs for nearly 3 hours and should never happen, because of budget cuts.

  • Cause of Problem: There's a bug in the Build Rules. I forgot to check for x86 PRs. (It works only for x86_64 PRs) I'll go fix this:
    CI: Run CI Job other only for Simple x86 PR #14896

  • The RISC-V Bug that appears in the CI Build: It has been flagged in nuttx-dashboard.org. I reported it here:
    [BUG] rv-virt/citest: test_hello or test_pipe failed #14808

  • Can we fix the RISC-V Bug? I'm not quite sure why pytest, QEMU and expect failed. I'll have to dig inside. (Maybe write an article about it)

lupyuen added a commit to lupyuen2/wip-nuttx that referenced this pull request Nov 22, 2024
Presently, Simple x86 PRs will run All CI Checks (across all Architectures), as reported here: apache#14885 (comment)

This PR fixes the CI Build Rules, so that Simple x86 PRs will run only One Single CI Job: `other`.
@raiden00pl
Copy link
Contributor

@W-M-R is this PR correct ? In the description you write about x86_64 but you modified x86

xiaoxiang781216 pushed a commit that referenced this pull request Nov 22, 2024
Presently, Simple x86 PRs will run All CI Checks (across all Architectures), as reported here: #14885 (comment)

This PR fixes the CI Build Rules, so that Simple x86 PRs will run only One Single CI Job: `other`.
@W-M-R
Copy link
Contributor Author

W-M-R commented Nov 22, 2024

the description you write about x86_64 but you modified x86

Sorry,#14900 ,this is the real x86-64 modification, and this patch is x86
Fortunately, except for the error in the comment, the actual architecture is supported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: x86 Issues related to the x86 architecture Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants