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

Fixed mutex assert in armcc fopen and related memory leak #5526

Merged
merged 1 commit into from Nov 27, 2017

Conversation

Projects
None yet
7 participants
@geky
Member

geky commented Nov 17, 2017

armcc fopen allocated a mutex using the retargeted system-level _mutex_initialize function. Interestingly, malloc also uses this same _mutex_initialization function, which prevents a full solution relying on malloc. The solution previously implemented involved using the rtx mutex pool for the first 8 mutexes, then falling back on malloc.

The previous implementation relied on osMutexNew returning an error on out-of-memory. An unrelated change causes osMutexNew to instead assert (except for release mode). This meant if you exceed 8 system- level mutexes in armcc you will hit an assert. Since the filesystem code can call fopen an unlimited number of times, this is a problem.

Solution is to keep track of which static mutexes we've allocated, so we know before calling osMutexNew if we need to call malloc.

Also _mutex_free never deallocated the malloced mutexes, which would cause fopen to leak memory.

cc @c1728p9, @deepikabhavnani, @bulislaw

@geky geky referenced this pull request Nov 17, 2017

Merged

Add filesystem recovery tests #9

@0xc0170 0xc0170 requested review from bulislaw and deepikabhavnani Nov 20, 2017

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Nov 20, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 20, 2017

/morph build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 20, 2017

@geky jenkins CI fails with a reason Error: L6200E: Symbol _mutex_free multiply defined (by .build/K66F_armcc_minimal/mbed-os/rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_lib.o and .build/K66F_armcc_minimal/mbed-os/rtos/TARGET_CORTEX/mbed_boot.o). , please review

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Nov 20, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 20, 2017

Fixed mutex assert in armcc fopen and related memory leak
armcc fopen allocated a mutex using the retargeted system-level
_mutex_initialize function. Interestingly, malloc also uses this
same _mutex_initialization function, which prevents a full solution
relying on malloc. The solution previously implemented involved using
the rtx mutex pool for the first 8 mutexes, then falling back on
malloc.

The previous implementation relied on osMutexNew returning an error
on out-of-memory. An unrelated change causes osMutexNew to instead
assert (except for release mode). This meant if you exceed 8 system-
level mutexes in armcc you will hit an assert. Since the filesystem
code can call fopen an unlimited number of times, this is a problem.

Solution is to keep track of which static mutexes we've allocated, so
we know before calling osMutexNew if we need to call malloc.

Also _mutex_free never deallocated the malloced mutexes, which would
cause fopen to leak memory.

@geky geky force-pushed the geky:fix-armcc-fopen-mutex-malloc branch from faeefc5 to 7e45aee Nov 22, 2017

@geky

This comment has been minimized.

Member

geky commented Nov 22, 2017

It looks like the weak attributes on the mutex functions were broken in a recent update?
a03591d#diff-2a0d9db94100b24b62a37fcbb6b69bde

I brought them back so that this patch works

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 24, 2017

@geky Thanks for fixing the weak linkage !

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Nov 24, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 24, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 24, 2017

Build : SUCCESS

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

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 0xc0170 added ready for merge and removed needs: CI labels Nov 27, 2017

@theotherjimmy theotherjimmy merged commit 33c9726 into ARMmbed:master Nov 27, 2017

6 checks passed

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment