applications: sdp: Add asm_gen target #17638

jaz1-nordic · 2024-10-04T08:09:47Z

Added asm_gen target, which is needed to generate assembler files for the hard real time part.

NordicBuilder · 2024-10-04T08:11:05Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 13

Inputs:

Sources:

sdk-nrf: PR head: d2efaacdfc7529e14dfa9b47f0a5ed8711218e72

more details

sdk-nrf:

PR head: d2efaacdfc7529e14dfa9b47f0a5ed8711218e72
merge base: 370f44ffed4f4199b375cf80c276296e5361e68c
target head (main): 5495fab81c5d0d27a8be74b3762bf8577f5504ed
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (6)

CMakeLists.txt
CODEOWNERS
applications
│  ├── sdp
│  │  ├── gpio
│  │  │  ├── CMakeLists.txt
│  │  │  ├── src
│  │  │  │  ├── hrt
│  │  │  │  │  │ hrt.c
cmake
│  │ sdp.cmake
scripts
│  ├── sdp
│  │  │ remove_comments.py

Outputs:

Toolchain

Version: 3dd8985b56
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:3dd8985b56_912848a074

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister - Skipped: Skipping Build & Test as it succeeded in a previous run: 8
✅ Integration tests
- ✅ test-sdk-audio - Skipped: Job was skipped as it succeeded in a previous run
- ✅ desktop52_verification - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-boot - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-apps - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test_ble_nrf_config - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-ble_mesh - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-ble_samples - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-chip - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nfc - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_cloud - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_libmodem-nrf - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_serial_lte_modem - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_zephyr_lwm2m - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_samples - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_lwm2m - Skipped: Job was skipped as it succeeded in a previous run
- ✅ doc-internal - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_thingy91 - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf_crypto - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-proprietary_esb - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-rpc - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-rs - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-fem - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-tfm - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-thread - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-zigbee - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-sdk-find-my - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_mosh - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_positioning - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-sdk-sidewalk - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-sdk-wifi - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-low-level - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_nrf_provisioning - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-sdk-pmic-samples - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-sdk-mcuboot - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-sdk-dfu - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-ps - Skipped: Job was skipped as it succeeded in a previous run

Note: This message is automatically posted and updated by the CI

NordicBuilder · 2024-10-04T08:25:57Z

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

cmake/sdp.cmake

scripts/sdp/clean_asm.py

tejlmand · 2024-10-09T10:39:06Z

cmake/sdp.cmake

+
+  zephyr_include_directories(include)
+
+  add_custom_target(asm_gen


please specify BYPRODUCTS.

Also, I can see this create a custom asm_gen target, but afaict there is no use of this target / its byproducts in the build system, and I see no documentation describing the intention or usage of this target.

How / when is this target intended to be invoked ?

This is just a part of a larger work related to generating assembler files for the Hard-Real Time part of SDP. This PR only aims to add a custom target in the form of asm_gen, which will be used in subsequent PRs.

please let the original commenter decide if the comment has been resolved.

This is not done:

please specify BYPRODUCTS.

also, regarding this:

This is just a part of a larger work related to generating assembler files for the Hard-Real Time part of SDP.

That might be, but without corresponding code I cannot review this function in depth, namely I cannot verify incremental builds or check that byproducts are correctly removed on a ninja clean.

If I approve this PR and later discover issues with the code in your next PR which uses the function , then that just results in a reply to me that, but this function was approved in PR#17638 (talking by experience).

So I need at least the follow up / usage of this code to be able to verify the full behavior of this code.

@tejlmand here is my draft PR that uses asm_gen: #18018. The intention is to run asm_check either in west build after the CMake configuration and before the building starts, or directly in CMake at the beginning of a build. We want to run asm_check in every SDP build, so that if someone changes the hrt/.c file, they know that the ASM file should be updated (the .c file will not be included in the build, it only serves as a source to generate the ASM file. The ASM file will be included in the build instead). We do not want to update the ASM file by default because it may change solely because a different compiler was used - which is what we are trying to avoid here.

Please, note that my PR is still a work in progress, thus calling asm_check is not added yet.

Also, if anything is done incorrectly in this PR, it is most likely also done incorrectly in mine, as I was basing my work on it.

cmake/sdp.cmake

scripts/sdp/clean_asm.py

cmake/sdp.cmake

masz-nordic · 2024-10-18T11:10:19Z

@tejlmand can you take a look?

cmake/sdp.cmake

tejlmand · 2024-10-18T15:08:10Z

scripts/sdp/clean_asm.py

I lack the reason for this clean_asm.py file.

Why is it important to remove all lines starting with a # in the assembly files ?

The files generated by the compilation proper step (-S) should always generate valid assembly files.
If you're trying to handle badly written user files, then perhaps it's better to ensure they write proper asm files in first place instead of outsmart as.

Perhaps check the docs regarding binutils as.

https://sourceware.org/binutils/docs/as/ARM_002dChars.html
https://sourceware.org/binutils/docs/as/Comments.html
https://sourceware.org/binutils/docs/as/Preprocessing.html

More information needed regarding the purpose.

The reason is that an assembly file will include full path to original files in some of the comments.
Those are unique to the user's environment and would come up as a diff when comparing functionally identical assembly files.

The reason is that an assembly file will include full path to original files in some of the comments.

do you have any examples ?
Generally there should not be comments starting with # ... when only doing the compilation proper (-S).
Have you perhaps been using -E during local local testing ?
Using -E (without -P) will stop right after pre-processing.
Note -P will not create line markers.

#17638 (comment)

scripts/sdp/clean_asm.py

cmake/sdp.cmake

applications/sdp/gpio/CMakeLists.txt

jaz1-nordic · 2024-10-21T15:35:53Z

@tejlmand I addressed your comments. Please check it out as soon as possible.

magp-nordic · 2024-10-22T09:39:50Z

I've noticed that hrt.s file was removed in the recent push. Was it intentional?

jaz1-nordic · 2024-10-22T10:24:32Z

I've noticed that hrt.s file was removed in the recent push. Was it intentional?

Yes, because this is just the output file from the sample hrt.c and is not needed in the repo. The target files will be uploaded with this PR:
#17984

tejlmand

Thanks for the update.

This looks much better 👍

But i'm still not convinced on the need for remove_comments.py script.
I have a feeling the issue you might have seen has been due to using -E instead -S during local testing / development, and when using -E you have been omitting -P.

Note, the below sub-comment is not directly related to this PR, and thus us not blocking approval of the PR.

Note that any Kconfig optimization change, for example -DCONFIG_SPEED_OPTIMIZATIONS=y or -DCONFIG_DEBUG_OPTIMIZATIONS=y will impact the generated assembly file, but if the assembly file is intended for hard real time support, then I don't think that is desirable.

And in such cases, as well as situations where an assembly file has further been hand optimized, then I don't think it's wise to always be printed a warning that you cannot turn off, as it currently is presented in #18018.

tejlmand · 2024-10-22T13:39:09Z

cmake/sdp.cmake

+
+function(sdp_assembly_generate hrt_srcs)
+  set(hrt_msg "Generating ASM files for Hard Real Time files.")
+  set(hrt_opts -g0 -P -fno-ident -fno-verbose-asm)


why -P ?
You are stopping after compiler proper, not after the preprocessor, so there should be no line markers in the assemply.
Ref: https://gcc.gnu.org/onlinedocs/gcc-14.2.0/gcc/Preprocessor-Options.html

-P Inhibit generation of linemarkers in the output from the preprocessor. This might be useful when running the preprocessor on something that is not C code, and will be sent to a program which might be confused by the linemarkers.

To generate asm output we use the -S option, not -E.
I also removed the -P because the code looks the same with and without it, and the comments are still there:

If you can suggest something that will allow me to remove these comments from the asm file without having to use a Python script, I would be grateful.

To generate asm output we use the -S option, not -E.

yes, I know, I was just curious if you during development had been using -E, in which case you normally see the # <lineno> "<path>"
It's common to see people experiencing issues during development with certain flags, and thus introducing fixes.
Fixes which later becomes obsolete when later changes are introduced.

So was trying to avoid such case.

Are you using the #APP and #NO_APP identifiers in the original source file ?
Will ping ofline for more into.

seems to be caused by inline assembly.
Please note that not every version of gcc will use #, you can also risk a @, for example like this:

@ 14 "/projects/github/review/nrf/applications/sdp/gpio/src/hrt/hrt.c" 1 .align 1 @ 0 "" 2

current investigation has found no options to prevent gcc from placing a file comment in generated assembly.

One risk with the post comment removal is the fact that gcc can use # or @ as comment identifier.
One factor impacting gcc's choice seems to be the -std=<std> in use.

An alternative is to invoke the compiler from the source folder or west workspace, and then use a relative path to the source file.

This can be done by setting WORKING_DIRECTORY <dir> and adjust the input source path accordingly.
This can result in generated output like:

@ 14 "hrt.c" 1 .align 1 @ 0 "" 2

or

@ 14 "nrf/applications/sdp/gpio/src/hrt/hrt.c" 1 .align 1 @ 0 "" 2

but for this PR, such change is not requested.
It's better to do such alternative in future followup.

tejlmand · 2024-10-22T13:42:04Z

cmake/sdp.cmake

+      message(FATAL_ERROR "sdp_assembly_generate() was called on a directory")
+    endif()
+    get_filename_component(src_filename ${hrt_src} NAME_WE)  # filename without extension
+    add_custom_command(TARGET asm_gen


this will be a problem if the sdp_assembly_generate() function is called multiple times, for example like this:

sdp_assembly_generate(foo.c) sdp_assembly_generate(bar.c)

because there can only be a single asm_gen target created.

This might be an acceptable limitation for now, but could you please make this check and provide a use-ful error to the user, for example a check like this:

if(TARGET asm_gen) message(FATAL_ERROR "sdp_assembly_generate() already called, please note that sdp_assembly_generate() must be called only once with a list of all source files." ) endif()

Note, line length not checked in above example.

The message with FATAL_ERROR has been added.

Added asm_gen target, which is needed to generate assembler files for the hard real time part. Signed-off-by: Jakub Zymelka <[email protected]>

tejlmand

approving.

Not fond of the current post-process as I'm concerned we will see issues / corner-cases in future. See #17638 (comment).

But approach is approved for now, but do create follow ticket for future improvements.

tejlmand · 2024-10-23T09:36:06Z

scripts/sdp/remove_comments.py

+    # Filter out lines starting with #
+    cleaned_lines = [line for line in lines if not line.startswith('#')]


note, gcc may use @ instead.
See #17638 (comment)

jaz1-nordic requested a review from magp-nordic October 4, 2024 08:09

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 4, 2024

jaz1-nordic requested a review from masz-nordic October 4, 2024 08:10

jaz1-nordic force-pushed the nrfx-6376_asm_gen branch 2 times, most recently from 6b53169 to 8f89e65 Compare October 4, 2024 14:08

jaz1-nordic marked this pull request as ready for review October 4, 2024 14:09

jaz1-nordic requested review from a team as code owners October 4, 2024 14:09

masz-nordic reviewed Oct 8, 2024

View reviewed changes

cmake/sdp.cmake Outdated Show resolved Hide resolved

masz-nordic added this to the 2.8.0 milestone Oct 9, 2024

tejlmand requested changes Oct 9, 2024

View reviewed changes

jaz1-nordic force-pushed the nrfx-6376_asm_gen branch from 8f89e65 to 79cad1c Compare October 11, 2024 10:28

jaz1-nordic requested a review from tejlmand October 11, 2024 12:24

magp-nordic approved these changes Oct 11, 2024

View reviewed changes

jaz1-nordic requested a review from masz-nordic October 11, 2024 13:46

masz-nordic reviewed Oct 11, 2024

View reviewed changes

scripts/sdp/clean_asm.py Outdated Show resolved Hide resolved

magp-nordic reviewed Oct 11, 2024

View reviewed changes

cmake/sdp.cmake Outdated Show resolved Hide resolved

masz-nordic approved these changes Oct 15, 2024

View reviewed changes

jaz1-nordic force-pushed the nrfx-6376_asm_gen branch from 79cad1c to 0385e0d Compare October 15, 2024 15:46

jaz1-nordic requested a review from a team as a code owner October 15, 2024 15:46

jaz1-nordic force-pushed the nrfx-6376_asm_gen branch from 0385e0d to 628448e Compare October 15, 2024 15:55

github-actions bot removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 15, 2024

masz-nordic approved these changes Oct 16, 2024

View reviewed changes

magp-nordic approved these changes Oct 16, 2024

View reviewed changes

jaz1-nordic force-pushed the nrfx-6376_asm_gen branch from 628448e to e29c043 Compare October 17, 2024 08:29

masz-nordic mentioned this pull request Oct 17, 2024

applications: sdp: gpio: add Hard RealTime IRQ code #17984

Merged

tejlmand requested changes Oct 18, 2024

View reviewed changes

magp-nordic mentioned this pull request Oct 18, 2024

Nrfx 6388 assembly mgmt west build #18018

Merged

jaz1-nordic force-pushed the nrfx-6376_asm_gen branch from e29c043 to 9a02543 Compare October 21, 2024 15:30

masz-nordic requested a review from tejlmand October 21, 2024 15:37

magp-nordic approved these changes Oct 22, 2024

View reviewed changes

tejlmand requested changes Oct 22, 2024

View reviewed changes

applications: sdp: Add asm_gen target

d2efaac

Added asm_gen target, which is needed to generate assembler files for the hard real time part. Signed-off-by: Jakub Zymelka <[email protected]>

jaz1-nordic force-pushed the nrfx-6376_asm_gen branch 2 times, most recently from 763b935 to d2efaac Compare October 22, 2024 17:51

carlescufi requested a review from tejlmand October 23, 2024 09:18

tejlmand approved these changes Oct 23, 2024

View reviewed changes

rlubos merged commit 5ede0d6 into nrfconnect:main Oct 23, 2024
13 checks passed

jaz1-nordic deleted the nrfx-6376_asm_gen branch October 23, 2024 11:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

applications: sdp: Add asm_gen target #17638

applications: sdp: Add asm_gen target #17638

jaz1-nordic commented Oct 4, 2024

NordicBuilder commented Oct 4, 2024 •

edited

Loading

NordicBuilder commented Oct 4, 2024

tejlmand Oct 9, 2024

jaz1-nordic Oct 11, 2024

tejlmand Oct 18, 2024

magp-nordic Oct 18, 2024

masz-nordic commented Oct 18, 2024

tejlmand Oct 18, 2024

masz-nordic Oct 21, 2024

tejlmand Oct 22, 2024

jaz1-nordic Oct 23, 2024

jaz1-nordic commented Oct 21, 2024 •

edited

Loading

magp-nordic commented Oct 22, 2024

jaz1-nordic commented Oct 22, 2024

tejlmand left a comment

tejlmand Oct 22, 2024

jaz1-nordic Oct 22, 2024 •

edited

Loading

tejlmand Oct 23, 2024

tejlmand Oct 23, 2024

tejlmand Oct 23, 2024 •

edited

Loading

tejlmand Oct 22, 2024

jaz1-nordic Oct 22, 2024

tejlmand left a comment

tejlmand Oct 23, 2024


		zephyr_include_directories(include)

		add_custom_target(asm_gen

		# Filter out lines starting with #
		cleaned_lines = [line for line in lines if not line.startswith('#')]

applications: sdp: Add asm_gen target #17638

applications: sdp: Add asm_gen target #17638

Conversation

jaz1-nordic commented Oct 4, 2024

NordicBuilder commented Oct 4, 2024 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

NordicBuilder commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masz-nordic commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaz1-nordic commented Oct 21, 2024 • edited Loading

magp-nordic commented Oct 22, 2024

jaz1-nordic commented Oct 22, 2024

tejlmand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaz1-nordic Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NordicBuilder commented Oct 4, 2024 •

edited

Loading

jaz1-nordic commented Oct 21, 2024 •

edited

Loading

jaz1-nordic Oct 22, 2024 •

edited

Loading

tejlmand Oct 23, 2024 •

edited

Loading