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

tools: check that part size is not exceeding region size #9021

Merged
merged 2 commits into from Feb 7, 2019

Conversation

Projects
None yet
8 participants
@naveenkaje
Copy link
Contributor

naveenkaje commented Dec 7, 2018

Description

Check that the Intel hex file segment fits within the region. Add a test to the tools test suite to verify the same.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@theotherjimmy

@naveenkaje

This comment has been minimized.

Copy link
Contributor Author

naveenkaje commented Dec 7, 2018

Addressing #9012 (comment)

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Dec 8, 2018

@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Dec 8, 2018

@naveenkaje, thank you for your changes.
@ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

@theotherjimmy
Copy link
Contributor

theotherjimmy left a comment

part.maxaddr may be past the end of ROM, as introduced by #8097

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

theotherjimmy commented Dec 10, 2018

OOps, got to give you a path forward. Support that use case please.

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Dec 21, 2018

@naveenkaje Has any progress been made on this PR?

@naveenkaje

This comment has been minimized.

Copy link
Contributor Author

naveenkaje commented Jan 3, 2019

Will make changes and upload.

@naveenkaje naveenkaje force-pushed the naveenkaje:fix_part_size_check branch to 2e74b8b Jan 7, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Jan 16, 2019

Ready for another review (it was updated)

@0xc0170 0xc0170 added needs: review and removed needs: work labels Jan 16, 2019

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

theotherjimmy commented Jan 16, 2019

@naveenkaje Would it be possible to add some tests for merge_region_list?

@cmonr cmonr added needs: work and removed needs: review labels Jan 16, 2019

@naveenkaje

This comment has been minimized.

Copy link
Contributor Author

naveenkaje commented Jan 24, 2019

@theotherjimmy Here are logs from a test in which target.restrict_size is set to overflow and with this change, such a condition is caught.

Test Log:

Configuration in mbed_app.json
"DISCO_L476VG": {
    "target.restrict_size": "0x15690"
}
Build log

Using ROM regions application, post_application in this build.
  Region application: size 0x15000, offset 0x8000000
  Region post_application: size 0xeb000, offset 0x8015000
Compile [  0.1%]: except.S
Compile [  0.2%]: mbed_tz_context.c
Compile [  0.4%]: MCR20Drv.c
Compile [  0.5%]: rf_configuration.c
Compile [  0.6%]: fslittle_test.c
...
Compile [ 99.8%]: stm_spi_api.c
Compile [ 99.9%]: us_ticker.c
Compile [100.0%]: trng_api.c
Link: mbed-os-example-blinky_application
Elf2Bin: mbed-os-example-blinky_application
Merging Regions
  Filling region application with ./BUILD/DISCO_L476VG/GCC_ARM-DEBUG/mbed-os-example-blinky_application.bin
[ERROR] Contents of region application does not fit
[mbed] ERROR: "/usr/bin/python" returned error.
       Code: 1
       Path: "/home/navkaj01/code/mbed-os-example-blinky"
       Command: "/usr/bin/python -u /home/navkaj01/code/mbed-os-example-blinky/mbed-os/tools/make.py -t GCC_ARM -m DISCO_L476VG --profile debug --source . --build ./BUILD/DISCO_L476VG/GCC_ARM-DEBUG -c"
       Tip: You could retry the last command with "-v" flag for verbose output
---
@theotherjimmy

This comment has been minimized.

Copy link
Contributor

theotherjimmy commented Jan 25, 2019

@naveenkaje I meant some unit tests. That way we can have a quick sanity check and faster feedback in case anyone wants to refactor this code.

@naveenkaje naveenkaje force-pushed the naveenkaje:fix_part_size_check branch from 2e74b8b Jan 29, 2019

@naveenkaje

This comment has been minimized.

Copy link
Contributor Author

naveenkaje commented Jan 29, 2019

Test results

$ python -m pytest -v tools/test/build_api/build_api_test.py
========================================================= test session starts ==========================================================
platform linux2 -- Python 2.7.12, pytest-3.3.0, py-1.7.0, pluggy-0.6.0 -- /usr/bin/python                                               
cachedir: .cache                                                                                                                        
<snip>                                                                                    
plugins: hypothesis-3.88.3                                                                                                              
collected 8 items                                                                                                                       
                                                                                                                                        
tools/test/build_api/build_api_test.py::BuildApiTests::test_always_complete_build PASSED                                         [ 12%] 
tools/test/build_api/build_api_test.py::BuildApiTests::test_build_library_app_config PASSED                                      [ 25%] 
tools/test/build_api/build_api_test.py::BuildApiTests::test_build_library_no_app_config PASSED                                   [ 37%] 
tools/test/build_api/build_api_test.py::BuildApiTests::test_build_project_app_config PASSED                                      [ 50%] 
tools/test/build_api/build_api_test.py::BuildApiTests::test_build_project_no_app_config PASSED                                   [ 62%] 
tools/test/build_api/build_api_test.py::BuildApiTests::test_merge_region_no_fit PASSED                                           [ 75%] 
tools/test/build_api/build_api_test.py::BuildApiTests::test_prepare_toolchain_app_config PASSED                                  [ 87%] 
tools/test/build_api/build_api_test.py::BuildApiTests::test_prepare_toolchain_no_app_config PASSED                               [100%] 
                                                                                                                                        
======================================================= 8 passed in 1.25 seconds =======================================================
@theotherjimmy
Copy link
Contributor

theotherjimmy left a comment

The new test cannot fail: it catches the only exception of the function under test and does not assert.

Please add an assert.

tools/test/build_api/build_api_test.py Outdated
@patch('tools.build_api.Resources')
@patch('tools.build_api.mkdir')
@patch('os.path.exists')
@patch('tools.build_api.prepare_toolchain')

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2019

Contributor

I don't think you need this patch

tools/test/build_api/build_api_test.py Outdated
"""
Test that merge region fails as expected when part size overflows region size.
"""
with patch("tools.build_api.intelhex_offset") as _intelhex_offset:

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2019

Contributor

Could you move this patch into the @patch directorator section above the funnction?

tools/test/build_api/build_api_test.py Outdated
region_list = [region_application, region_post_application]
region_list = list(region_list)
toolchain = prepare_toolchain(self.src_paths, self.build_path, self.target,
self.toolchain_name, notify=notify)

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2019

Contributor

You don't need a toolchain object at all, so you can delete this line and the line above

tools/test/build_api/build_api_test.py Outdated
mock_prepare_toolchain.config = MagicMock(
has_regions=True, name=None, lib_config_data=None)
region_list = [region_application, region_post_application]
region_list = list(region_list)

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2019

Contributor

This line is not needed, as region_list is already a list.

tools/test/build_api/build_api_test.py Outdated
@@ -238,5 +240,41 @@ def test_build_library_no_app_config(self, mock_prepare_toolchain, mock_exists,
self.assertEqual(args[1]['app_config'], None,
"prepare_toolchain was called with an incorrect app_config")

@patch('tools.build_api.Resources')
@patch('tools.build_api.mkdir')
@patch('os.path.exists')

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2019

Contributor

os.path.exsits is not used, so you don't need this patch either.

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

theotherjimmy commented Jan 29, 2019

Come to think of it, you probably don't need to patch tools.build_api.Resources or tools.build_api.mkdir, either.

@naveenkaje naveenkaje force-pushed the naveenkaje:fix_part_size_check branch Jan 29, 2019

tools/test/build_api/build_api_test.py Outdated

notify = MockNotifier()
region_list = [region_application, region_post_application]
toolchain = prepare_toolchain(self.src_paths, self.build_path, self.target,

This comment has been minimized.

@naveenkaje

naveenkaje Jan 29, 2019

Author Contributor

toolchain needed to pass to merge_region_list()

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2019

Contributor

merge_region_list(region_list, res, notify, toolchain.config)

Nope. It needs a config object, or stand in. That's not a toolchain object.

This comment has been minimized.

@naveenkaje

naveenkaje Jan 29, 2019

Author Contributor

Thanks @theotherjimmy got it. Updated the patch.

@naveenkaje naveenkaje force-pushed the naveenkaje:fix_part_size_check branch Jan 29, 2019

@theotherjimmy
Copy link
Contributor

theotherjimmy left a comment

Improving, but we still don't assert any behavior of merge_region_list. Please add an assert to the return value or in either the try or the except path, enforcing that only one of them is possible.

tools/test/build_api/build_api_test.py Outdated
mock_intelhex_offset.return_value = IntelHex({0:2, max_addr:0})

self.assertEqual(0, mock_intelhex_offset.return_value.minaddr())
self.assertEqual(max_addr, mock_intelhex_offset.return_value.maxaddr())

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2019

Contributor

These asserts don't test any tools code.

This comment has been minimized.

@naveenkaje

naveenkaje Jan 29, 2019

Author Contributor

since we use the IntelHex() library to generate this object, we can forego these asserts?

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2019

Contributor

Yes.

@naveenkaje naveenkaje force-pushed the naveenkaje:fix_part_size_check branch Jan 29, 2019

@naveenkaje

This comment has been minimized.

Copy link
Contributor Author

naveenkaje commented Jan 29, 2019

Without fix 060d3d3

$ python -m pytest -v tools/test/build_api/build_api_test.py                                          │
======================================================== test session starts ========================================================│
platform linux2 -- Python 2.7.12, pytest-3.3.0, py-1.7.0, pluggy-0.6.0 -- /usr/bin/python                                            │
cachedir: .cache                                                                                                                     │
<snip>
plugins: hypothesis-3.88.3                                                                                                           │
collected 8 items                                                                                                                    │
                                                                                                                                     │
tools/test/build_api/build_api_test.py::BuildApiTests::test_always_complete_build PASSED                                      [ 12%] │
tools/test/build_api/build_api_test.py::BuildApiTests::test_build_library_app_config PASSED                                   [ 25%] │
tools/test/build_api/build_api_test.py::BuildApiTests::test_build_library_no_app_config PASSED                                [ 37%] │
tools/test/build_api/build_api_test.py::BuildApiTests::test_build_project_app_config PASSED                                   [ 50%] │
tools/test/build_api/build_api_test.py::BuildApiTests::test_build_project_no_app_config PASSED                                [ 62%] │
tools/test/build_api/build_api_test.py::BuildApiTests::test_merge_region_no_fit FAILED                                        [ 75%] │
tools/test/build_api/build_api_test.py::BuildApiTests::test_prepare_toolchain_app_config PASSED                               [ 87%] │
tools/test/build_api/build_api_test.py::BuildApiTests::test_prepare_toolchain_no_app_config PASSED                            [100%] │
                                                                                                                                     │
============================================================= FAILURES ==============================================================│
______________________________________________ BuildApiTests.test_merge_region_no_fit ______________________________________________

With fix 060d3d3

======================================================== test session starts ========================================================│
platform linux2 -- Python 2.7.12, pytest-3.3.0, py-1.7.0, pluggy-0.6.0 -- /usr/bin/python                                            │
cachedir: .cache                                                                                                                     │
<snip>
plugins: hypothesis-3.88.3                                                                                                           │
collected 8 items                                                                                                                    │
                                                                                                                                     │
tools/test/build_api/build_api_test.py::BuildApiTests::test_always_complete_build PASSED                                      [ 12%] │
tools/test/build_api/build_api_test.py::BuildApiTests::test_build_library_app_config PASSED                                   [ 25%] │
tools/test/build_api/build_api_test.py::BuildApiTests::test_build_library_no_app_config PASSED                                [ 37%] │
tools/test/build_api/build_api_test.py::BuildApiTests::test_build_project_app_config PASSED                                   [ 50%] │
tools/test/build_api/build_api_test.py::BuildApiTests::test_build_project_no_app_config PASSED                                [ 62%] │
tools/test/build_api/build_api_test.py::BuildApiTests::test_merge_region_no_fit PASSED                                        [ 75%] │
tools/test/build_api/build_api_test.py::BuildApiTests::test_prepare_toolchain_app_config PASSED                               [ 87%] │
tools/test/build_api/build_api_test.py::BuildApiTests::test_prepare_toolchain_no_app_config PASSED                            [100%] │
                                                                                                                                     │
===================================================== 8 passed in 1.30 seconds ======================================================│

@naveenkaje naveenkaje force-pushed the naveenkaje:fix_part_size_check branch 2 times, most recently Jan 30, 2019

tools: check part size is not exceeding region size
If config is specified, check that part size is not
exceeding the region. Normally we now assume that
part.maxaddr() can be beyond end of rom.

@naveenkaje naveenkaje force-pushed the naveenkaje:fix_part_size_check branch Jan 31, 2019

@naveenkaje naveenkaje force-pushed the naveenkaje:fix_part_size_check branch to 43da2f2 Jan 31, 2019

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Feb 4, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Feb 4, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Feb 4, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 1
Build artifacts

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

NirSonnenschein commented Feb 5, 2019

@0xc0170 CI passed, does this need another review?

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Feb 5, 2019

@0xc0170

0xc0170 approved these changes Feb 5, 2019

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

NirSonnenschein commented Feb 6, 2019

@naveenkaje , please restore the template to the PR description and fill in.

@adbridge adbridge merged commit 645c1ce into ARMmbed:master Feb 7, 2019

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9715 cycles (+483 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.