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

ARMC5+6: Specify CPU for ARM scatter file preprocessor #5797

Merged
merged 5 commits into from Feb 7, 2018

Conversation

Projects
None yet
6 participants
@hosse005
Contributor

hosse005 commented Jan 5, 2018

Fixes #5796

Description

Fix for targets which include the c preprocessor in the ARM and ARMC6 scatter file.

Status

READY

Migrations

NO

Steps to test or reproduce

  1. Pull the mbed-os-example-blinky app
  2. Import the latest mbed-os (>= mbed-os-5.5.4)
  3. Compile with either the ARM or ARMC6 compiler and target the MultiTech xDot
    (e.g. mbed compile -t ARMC6 -m xdot_l151cc)
  4. Verify the app builds
@hosse005

This comment has been minimized.

Contributor

hosse005 commented Jan 5, 2018

@0xc0170 0xc0170 requested a review from theotherjimmy Jan 6, 2018

@theotherjimmy

Looks good. Thanks!

@hosse005

This comment has been minimized.

Contributor

hosse005 commented Jan 8, 2018

Looks like this is related to issue #5421 as well, but this solution is giving some problems with the k64f target saying ARM Compiler does not support '-mcpu=cortex-m4f' just for the arm 6 compiler. Maybe some additional string massaging needed here?

@theotherjimmy theotherjimmy changed the title from ARM: ARMC6: Specify CPU for ARM scatter file preprocessor to ARMC6: Specify CPU for ARM scatter file preprocessor Jan 8, 2018

@theotherjimmy theotherjimmy changed the title from ARMC6: Specify CPU for ARM scatter file preprocessor to ARMC5+6: Specify CPU for ARM scatter file preprocessor Jan 8, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 8, 2018

@hosse005

This comment has been minimized.

Contributor

hosse005 commented Jan 8, 2018

@0xc0170 Yes, I just accepted

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 9, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 9, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 9, 2018

Build failed for one target, RZ_A1H, with the error [DEBUG] Errors: Error: L6636E: Pre-processor step failed for '/builds/ws/mbed-os-build-matrix/826/RZ_A1H_ARM/sources/mbed-os/BUILD/tests/RZ_A1H/ARM/./TESTS/netsocket/udp_echo/.link_script.sct' it complains about not finding a header file earlier in the log, can you please verify?

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Jan 9, 2018

@hosse005

This comment has been minimized.

Contributor

hosse005 commented Jan 9, 2018

My license doesn't allow me to compile for an A9, so my help maybe limited here. Are we sure this ci job doesn't fail without these changes? Maybe the "mem_RZ_A1H.h" file isn't being copied into the build directory along with the MBRZA1H.sct file?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 10, 2018

I could reproduce it. The same linker failure. Master is OK, it's only this branch.

        [DEBUG] Errors: Error: L6636E: Pre-processor step failed for 'mbed-os/BUILD/tests/RZ_A1H/ARM/./TESTS/mbedmicro-rtos-mbed/mutex/.link_script.sct'

This is the top of the link_script.sct

#! armcc -E --cpu=Cortex-A9
;**************************************************
; Copyright (c) 2017 ARM Ltd.  All rights reserved.
;**************************************************

; Scatter-file for RTX Example on Versatile Express

; This scatter-file places application code, data, stack and heap at suitable addresses in the memory map.

#include "mem_RZ_A1H.h"

The only change when I compare those 2 is --cpu flag that is being added here. In the build dir, the header file is there. Could this cpu flag affect include dirs? I checked include dirs that are in the build file, the path is there for that header (in the include paths). Also in the build dir, it is present.

Why this cpu flag is added to the linker script file and not into .profile-asm ? (the cpu flag for C is in the .profile-c file). Does not help thought with this, fails anyway the same way.

ARM: ARMC6: Copy headers along with the updated linker scatter file
* Need to copy headers into the build directory as well when also
  writing an updated linker scatter file to the build directory
@hosse005

This comment has been minimized.

Contributor

hosse005 commented Jan 10, 2018

I was able to get a hold of an eval license so I could reproduce the issue. I don't think headers were getting copied over from the script source directory when we needed to update the scatter file shebang. I made some change which should address this issue.

@0xc0170 , I added the cpu arg to the scatter file shebang because this is where I was seeing compile errors coming from, and it seemed to be needed at least for ARMC6 according to the ARM compiler v5 -> v6 migration app note (http://www.keil.com/appnotes/files/apnt_298.pdf)

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 11, 2018

@0xc0170 , I added the cpu arg to the scatter file shebang because this is where I was seeing compile errors coming from, and it seemed to be needed at least for ARMC6 according to the ARM compiler v5 -> v6 migration app note (http://www.keil.com/appnotes/files/apnt_298.pdf)

👍 for the reference

I was able to get a hold of an eval license so I could reproduce the issue. I don't think headers were getting copied over from the script source directory when we needed to update the scatter file shebang. I made some change which should address this issue.

I was checking locally and to me the header files were there? I still have the build locally, BUILD\tests\RZ_A1H\ARM\targets\TARGET_RENESAS\TARGET_RZ_A1H\device\TOOLCHAIN_ARM_STD\ this is where mem_RZ_A1H header file is. What am I missing ?

@hosse005

This comment has been minimized.

Contributor

hosse005 commented Jan 11, 2018

Hmm.., @0xc0170 can you try to replicate my test setup? I am just building the mbed-os-example-blinky app with the ARM compiler targeting the RZ_A1H with the mbed-os changes in this PR. Without the last commit I see the same failures observed from the ci job logs, and with the last change it is resolved.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 12, 2018

I tested this fix with blinky , and also mutex test, both compiles on my machine.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 12, 2018

/morph build

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Jan 12, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 12, 2018

Build : SUCCESS

Build number : 848
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5797/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@hosse005

This comment has been minimized.

Contributor

hosse005 commented Jan 23, 2018

I think abspath maybe needed here for the build machine as follows:
self.SHEBANG += " -I %s" % dirname(abspath(scatter_file))

@theotherjimmy , do you agree?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jan 23, 2018

@hosse005 I do not. When you export a project from the online compile, an absolute path will always be incorrect.

We will have to make the path relative to the project file location, so relpath(scatter_file, self.build_dir) may be necessary

@mbed-ci

This comment has been minimized.

@cmonr cmonr added needs: work and removed needs: CI labels Jan 23, 2018

@hosse005

This comment has been minimized.

Contributor

hosse005 commented Jan 24, 2018

@theotherjimmy , I think just relpath(dirname(scatter_file)) should work. The linker isn't executing from the build directory directly so that failed for me, but the above code relative to the current working directory worked for me, just not sure about the build machine? Thought I would ask first before pushing it up.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jan 24, 2018

@hosse005 I don't think that relpath(dirname(scatter_file)) is an option either. When exporting in the online compiler, the tools are not executed from a project root, and projects do not have a single root. Instead, the tools are given a list of directories and where these directories will be in the final zip file. This makes changes like this one quite complicated.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jan 24, 2018

I think the "right solution" will have to be relpath(dirnname(scatter_file), resources.file_basepath[scatter_file]), and the resources object will have to be passed in.

@hosse005

This comment has been minimized.

Contributor

hosse005 commented Jan 24, 2018

@theotherjimmy , I may need some help with this one then, not sure how to do this.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jan 31, 2018

@hosse005 I think that'll do everything I need it to; offline, online export and online compile. Sorry that took so long.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 1, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 1, 2018

Build : SUCCESS

Build number : 1038
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5797/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 1, 2018

Exporters fail as we saw earlier for Cortex A targets, please review

restarting uvisor

/morph uvisor-test

@orenc17

This comment has been minimized.

Contributor

orenc17 commented Feb 1, 2018

/morph uvisor-test

@theotherjimmy theotherjimmy force-pushed the hosse005:master branch from 9f681e9 to 9b8ba4d Feb 2, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 3, 2018

Dunno why results aren't being shown. Rebuilding.

/morph build

@cmonr cmonr added needs: CI and removed needs: work labels Feb 3, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 3, 2018

Build : SUCCESS

Build number : 1056
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5797/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels Feb 3, 2018

@cmonr cmonr merged commit 779ee84 into ARMmbed:master Feb 7, 2018

19 checks passed

ARM mbed CI Verification build successful.
Details
AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment