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

platform: make C++ allocation wrappers log the correct caller address #5456

Merged
merged 2 commits into from Jan 15, 2018

Conversation

Projects
None yet
7 participants
@TeroJaasko
Contributor

TeroJaasko commented Nov 8, 2017

Description

The C++ "operator new" and "operator delete" (and their array
variants) were logging the the caller address wrong. In practice
if one used "operator new", the logged caller address pointed
to mbed_retarget.cpp, not to the client. Fix this by exposing
the alloc wrappers to the the retarget.

Note: this fixes only the ARMCC variants, as the GCC ones have
different different API and implementation.

Related mbed-os issue: #5221

Status

IN DEVELOPMENT, GCC should also be fixed.

Migrations

NO

Related PRs

Todos

  • Tests
  • Documentation

Deploy notes

Note: the GCC target is equally broken, but this PR is not fixing that.

Steps to test or reproduce

Test code

class test_class
{
public:
    uint32_t member;
};

static void do_the_test()
{
    void *mem_malloc = malloc(100);
    void *mem_calloc = calloc(60, 2);
    test_class* cpp_new_obj = new test_class;
    uint8_t* cpp_new_array = new uint8_t[200];
    delete cpp_new_obj;
    delete[] cpp_new_array;
    free(mem_calloc);
    free(mem_malloc);
}

@TeroJaasko TeroJaasko force-pushed the TeroJaasko:fix_cpp_alloc_wrappers branch Nov 8, 2017

@0xc0170 0xc0170 requested review from pan- and c1728p9 Nov 8, 2017

@pan-

pan- approved these changes Nov 8, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 9, 2017

Note: the GCC target is equally broken, but this PR is not fixing that.

@TeroJaasko Can we fix also GCC in this patch, or are there any blockers ?

this fixes only the ARMCC variants

I can see also IAR is fixed. In the code please can you change TOOLCHAIN_ARM to the symbol that ARMCC provides?

@TeroJaasko

This comment has been minimized.

Contributor

TeroJaasko commented Nov 9, 2017

@0xc0170: GCC variant has those "struct _reent" parameters in the wrappers, eg. "__real__malloc_r(struct _reent * r, size_t size)". I have no idea where to get that from C++ side. Any pointers where to look?

The IAR build does not seem to get the caller address at all, as the MBED_CALLER_ADDRESS macro is defined as NULL, see https://github.com/ARMmbed/mbed-os/blob/master/platform/mbed_toolchain.h#L314. Is there a way to get the caller address on IAR? Or should I just remove the
IAR part from ifdef?

Regarding TOOLCHAIN_ARM - which is the correct one to use instead of it? On the mbed_retarget.cpp there are __ARMCC_VERSION and __CC_ARM used in addition of TOOLCHAIN_ARM.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 16, 2017

@0xc0170: GCC variant has those "struct _reent" parameters in the wrappers, eg. "__real__malloc_r(struct _reent * r, size_t size)". I have no idea where to get that from C++ side. Any pointers where to look?

Don't know from top of my head, @pan- any pointers?

The IAR build does not seem to get the caller address at all, as the MBED_CALLER_ADDRESS macro is defined as NULL, see https://github.com/ARMmbed/mbed-os/blob/master/platform/mbed_toolchain.h#L314. Is there a way to get the caller address on IAR? Or should I just remove the
IAR part from ifdef?

Looks correct, not sure if this changes with IAR 8 update, I could not find it.

Regarding TOOLCHAIN_ARM - which is the correct one to use instead of it? On the mbed_retarget.cpp there are __ARMCC_VERSION and __CC_ARM used in addition of TOOLCHAIN_ARM.

__CC_ARM should be sufficient

@pan-

This comment has been minimized.

Member

pan- commented Nov 16, 2017

I have no idea where to get that from C++ side. Any pointers where to look?

You may include <reent.h> and use the macro _REENT but there is no guarantee this will work long term.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Nov 20, 2017

@TeroJaasko Could you fix this in all 3 compilers before we merge it?

@TeroJaasko TeroJaasko force-pushed the TeroJaasko:fix_cpp_alloc_wrappers branch to 758a026 Nov 24, 2017

@TeroJaasko

This comment has been minimized.

Contributor

TeroJaasko commented Nov 24, 2017

@pan- : thanks for guidance, that seems to work.
@theotherjimmy: Added the GCC support and changed the TOOLCHAIN_ARM to __CC_ARM.

IAR 8.x version seem to mostly work also if I use "#define MBED_CALLER_ADDR() ((void *)__get_LR())". I don't have IAR 7 installed, so if possible, could you take the ball on the IAR support?

@adbridge

This comment has been minimized.

Contributor

adbridge commented Dec 29, 2017

@TeroJaasko Could you please rebase this to remove the conflict ?

@TeroJaasko TeroJaasko force-pushed the TeroJaasko:fix_cpp_alloc_wrappers branch from 758a026 to a1bd296 Jan 4, 2018

@TeroJaasko

This comment has been minimized.

Contributor

TeroJaasko commented Jan 4, 2018

@adbridge : done. At same time, removed also the pointless "if (ptr)"'s from the operator delete wrappers as they were removed from the original versions.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 5, 2018

I restarted jenkins CI (network failure there)

@TeroJaasko what is outstanding here?

IAR 8.x version seem to mostly work also if I use "#define MBED_CALLER_ADDR() ((void *)__get_LR())". I don't have IAR 7 installed, so if possible, could you take the ball on the IAR support?

IAR support (version 7.x) ?

@TeroJaasko

This comment has been minimized.

Contributor

TeroJaasko commented Jan 8, 2018

As far as I know, the IAR target has never been able to print the caller address at all, just zeros. If you want to fix also IAR as part of this PR, please do so.

@0xc0170 0xc0170 added needs: review and removed needs: work labels Jan 8, 2018

@0xc0170 0xc0170 requested a review from SenRamakri Jan 8, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 8, 2018

Summarize: ARMCC and GCC fixed, IAR does not provide caller address thus no change there.

This fix is ready for review

@0xc0170

0xc0170 approved these changes Jan 8, 2018

TeroJaasko added some commits Nov 8, 2017

platform: make C++ allocation wrappers log the correct caller address
The C++ "operator new" and "operator delete" (and their array
variants) were logging the the caller address wrong. In practice
if one used "operator new", the logged caller address pointed
to mbed_retarget.cpp, not to the client. Fix this by exposing
the alloc wrappers to the the retarget.

Note: this fixes only the ARMCC variants, as the GCC ones have
different different API and implementation.
platform: Fix C++ allocation wrappers for GCC compilation
Fix the caller address logging on the GCC compilation too.
Previously the code logged the caller address as C++ wrapper,
not the actual caller of the C++ operator new or delete.

@TeroJaasko TeroJaasko force-pushed the TeroJaasko:fix_cpp_alloc_wrappers branch from a1bd296 to b81be1e Jan 8, 2018

@TeroJaasko

This comment has been minimized.

Contributor

TeroJaasko commented Jan 8, 2018

CI build pointed out that I had broken the non-tracing IAR build. Fixed it and force pushed.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 10, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 10, 2018

Build : SUCCESS

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

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

This comment has been minimized.

Contributor

cmonr commented Jan 11, 2018

@TeroJaasko Did you intend on adding any additional documentation?

@TeroJaasko

This comment has been minimized.

Contributor

TeroJaasko commented Jan 12, 2018

@cmonr : I have nothing to add here.

@0xc0170 0xc0170 merged commit 280d491 into ARMmbed:master Jan 15, 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

@TeroJaasko TeroJaasko deleted the TeroJaasko:fix_cpp_alloc_wrappers branch Jan 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment