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

Replace runtime strip_path function with compiler intrinsic equivalents #6377

Merged
merged 5 commits into from Mar 23, 2018

Conversation

Projects
None yet
7 participants
@scartmell-arm
Contributor

scartmell-arm commented Mar 15, 2018

Description

Replace runtime strip_path function with compiler intrinsic equivalents

Sleep manager tracing strips the path from filenames and uses the result as an identifier to track drivers that unlock/lock sleep tracing. Replace the function that strips the path from the string, replace this function with a new macro, __FILENAME__ which performs the same action in a compiler specific manner.

  • GCC_ARM, use __builtin_strrchr which is optimized out at compile time.
  • ARM, use __MODULE__ which returns the filename without a path.
  • IAR, specify the --no_path_in_file_macros compiler flag.

Change the driver identifier in the sleep tracing manager to a pointer of the stored file path.

Currently, the identifier is 15-byte character array that is copied from the __FILE__ of the driver being profiled. Given that the content of __FILE__ is stored inside the binary for the duration of the application it would be more efficient to store a pointer to this character string. This saves space in the statistics struct and saves time during lookup and insertion.

Pull request type

  • Fix
  • Refactor
  • New target
  • Feature
  • Breaking change

@deepikabhavnani @SenRamakri @bulislaw

Replace runtime strip_path function with compiler intrinsic equivalents
Sleep manager tracing strips the path from filenames and uses the result as an
identifier to track drivers that unlock/lock sleep tracing. Replace the function
that strips the path from the string, replace this function with a new macro,
__FILENAME__ which performs the same action in a compiler specific manner.

- GCC_ARM, use __builtin_strrchr which is optimized out at compile time.
- ARM, use __MODULE__ which returns the filename without path.
- IAR, specifiy the --no_path_in_file_macros compiler flag.
#if defined(__CC_ARM) || (defined(__ARMCC_VERSION) && (__ARMCC_VERSION >= 6010050))
#define __FILENAME__ __MODULE__
#elif defined(__GNUC__)
#define __FILENAME__ (__builtin_strrchr(__FILE__, '/') ? __builtin_strrchr(__FILE__, '/') + 1 : __builtin_strrchr(__FILE__, '\\') ? __builtin_strrchr(__FILE__, '\\') + 1 : __FILE__)

This comment has been minimized.

@geky

geky Mar 15, 2018

Member

Doesn't GCC map strchr to __builtin_strrchr automatically?

This comment has been minimized.

@scartmell-arm

scartmell-arm Mar 15, 2018

Contributor

Apparently yes, I just hadn't turned on optimizations when checking, it doesn't for -O0.

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Mar 15, 2018

Contributor

You can use __BASE_FILE__ for GNU. https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html
__BASE_FILE__
This macro expands to the name of the main input file, in the form of a C string constant. This is the source file that was specified on the command line of the preprocessor or C compiler.

This comment has been minimized.

@scartmell-arm

scartmell-arm Mar 15, 2018

Contributor

I tried it, but it doesn't work in this situation, it expands to the full path of the module that is passed into make/compiler so the output is the same as __FILE__ in this scenario, and something completely different in others.

Refactor sleep tracing driver identifier to be pointer to the driver …
…filepath.

The use of __FILE__ macro to get a usable identifier from the driver path
causes the path of the file to be stored in the .text region of the binary.
Given that this remains for the entire duration of the program, storing a
pointer to this string as an identifier is more efficient than copying the
contents of the string during lookup/insertion.

@scartmell-arm scartmell-arm force-pushed the scartmell-arm:feature-deep-sleep-tracing-filename-fix branch from e6b20ed to 9a0e879 Mar 15, 2018

sleep_manager_lock_deep_sleep_internal(); \
sleep_tracker_lock(__FILE__, __LINE__); \
} while (0);
#if defined(__CC_ARM) || (defined(__ARMCC_VERSION) && (__ARMCC_VERSION >= 6010050))

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Mar 15, 2018

Contributor

I guess we don't have to check version of ARM compiler here. __CC_ARM should be sufficient

@0xc0170

Looks fine, but the change --no_path_in_file_macros in the compiler flags for IAR - is this required for this patch to land?

Changing the compiler flags sets the minor version label for this

@scartmell-arm

This comment has been minimized.

Contributor

scartmell-arm commented Mar 19, 2018

I wouldn't say it's necessary, but removing it does change the behaviour between GCC/ARM and IAR.

Sleep locks held:
[id: Timer.cpp, count: 1]
[id: Ticker.h, count: 1]

// IAR
Sleep locks held:
[id: .\mbed-os\drivers\Timer.cpp, count: 1]
[id: D:\work\mbed-os-example-profiling\mbed-os\drivers/Ticker.h, count: 1]
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 19, 2018

Good snippet to see how it changes.

If compiler flag change can be send separately, this bugfix can go into patch release. Otherwise this will be labeled for the next minor release due to changing compiler flags.
@scartmell-arm Please advice

@scartmell-arm

This comment has been minimized.

Contributor

scartmell-arm commented Mar 19, 2018

That's fine, I've removed it so it can be added elsewhere.

sleep_tracker_lock(__FILE__, __LINE__); \
} while (0);
#if defined(__CC_ARM)
#define __FILENAME__ __MODULE__

This comment has been minimized.

@0xc0170

0xc0170 Mar 19, 2018

Member

Why is this starting with __ ? This resembles C lib macro but it is not ? As this is in platform header, I expect this goes to the user space (not internal use only).

If it is our own macro, should not use reserved characters (prefix ___). Similar to what we define in mbed toolchain (shouldn't this be there rather?)

// string literal.
#ifndef FILENAME
#if defined(__CC_ARM)
#define FILENAME __MODULE__

This comment has been minimized.

@0xc0170

0xc0170 Mar 20, 2018

Member

should it follow the same prefix MBED_FILENAME?

This comment has been minimized.

@scartmell-arm

scartmell-arm Mar 20, 2018

Contributor

yep, fixed

Fix issues with __FILENAME__ macro
- Move macro definition to mbed_toolchain.h
- Remove double underscores from macro which are reserved.
- Fix macro for IAR until compiler flags to disable path are added again.

@scartmell-arm scartmell-arm force-pushed the scartmell-arm:feature-deep-sleep-tracing-filename-fix branch from 5350f35 to ab2abcb Mar 20, 2018

@0xc0170 0xc0170 requested a review from bulislaw Mar 20, 2018

}

core_util_atomic_incr_u8(&stat->count, 1);

debug("LOCK: %s, ln: %i, lock count: %u\r\n", stripped_path, line, deep_sleep_lock);
debug("LOCK: %s, ln: %i, lock count: %u\r\n", filename, line, deep_sleep_lock);

This comment has been minimized.

@bulislaw

bulislaw Mar 20, 2018

Member

Do we really need the lock/unlock calls? I would say they just repeat the same info we get out of the who's holding sleep lock prints.

This comment has been minimized.

@scartmell-arm

scartmell-arm Mar 20, 2018

Contributor

Those changes are in a different PR: #6349

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 21, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: review labels Mar 21, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 22, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 22, 2018

/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 Mar 22, 2018

Hmm, looks like a UBLOX_EVK_ODIN_W2 timeouts.

/morph test

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit ddf70f1 into ARMmbed:master Mar 23, 2018

11 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build 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 Passed, code size is 10060B
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label Mar 23, 2018

@scartmell-arm scartmell-arm deleted the scartmell-arm:feature-deep-sleep-tracing-filename-fix branch Dec 5, 2018

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