-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
Heap: Improve debug caller address reporting #8720
Open
mhightower83
wants to merge
41
commits into
esp8266:master
Choose a base branch
from
mhightower83:pr-heap-refactor3
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 7 commits
Commits
Show all changes
41 commits
Select commit
Hold shift + click to select a range
4ec3c81
Lite refactoring and expanded comments
mhightower83 6088147
Improve caller address reporting to indicate address in
mhightower83 a716193
missed items
mhightower83 1102256
improve comments
mhightower83 6dfd46a
Merge branch 'master' into pr-heap-refactor3
mhightower83 8b3bb1f
Merge branch 'master' into pr-heap-refactor3
mhightower83 ea9befa
Finish merge cleanuo
mhightower83 3dd08be
requested changes
mhightower83 45f1c3b
Added missing DRAM fallback to pvPortCallocIram, pvPortZallocIram, and
mhightower83 57eb2b3
Add nothrow to aliases
mhightower83 c2590a7
Merge branch 'master' into pr-heap-refactor3
mhightower83 0619034
Merge branch 'pr-heap-refactor3' of github.com:mhightower83/Arduino i…
mhightower83 19da675
Merge branch 'master' into pr-heap-refactor3
mhightower83 af7b962
Merge branch 'pr-heap-refactor3' of github.com:mhightower83/Arduino i…
mhightower83 9423c8e
Merge branch 'master' into pr-heap-refactor3
mhightower83 471f838
Merge branch 'pr-heap-refactor3' of github.com:mhightower83/Arduino i…
mhightower83 d829540
Merge branch 'master' into pr-heap-refactor3
mhightower83 4c96365
Merge branch 'pr-heap-refactor3' of github.com:mhightower83/Arduino i…
mhightower83 840e55a
Merge branch 'master' into pr-heap-refactor3
mhightower83 95c7e2c
Merge branch 'master' into pr-heap-refactor3
mhightower83 0ce295e
Merge branch 'master' into pr-heap-refactor3
mhightower83 4717dcf
Use the same format for printing caller, file, line details
mhightower83 b083043
refactored print_loc()
mhightower83 ecc1b14
Merge branch 'master' into pr-heap-refactor3
mhightower83 a0044d6
Merge branch 'master' into pr-heap-refactor3
mhightower83 01b238a
Merge branch 'master' into pr-heap-refactor3
mhightower83 b5aa1de
removed unused variable from umm_local.c
mhightower83 1c99daf
Merge branch 'master' into pr-heap-refactor3
mhightower83 2bbadca
Merge branch 'master' into pr-heap-refactor3
mhightower83 49b48fb
Requested changes
mhightower83 3786ce6
Added missing "new" operators
mhightower83 b9a0e55
Added missing delete array operators
mhightower83 a5f6d7d
malloc align build cleanup - fully builds
mhightower83 54b034b
Correct possition of UMM_ENABLE_MEMALIGN handling in umm_malloc/umm_m…
mhightower83 e948329
remove static_assert checks on alignment
mhightower83 5653249
Updated changes to match upstream style using uncrustify.
mhightower83 d303d01
update comments
mhightower83 01a89fd
Added test Sketch for "new" and "delete" operators
mhightower83 0574c1a
Oops, remove external debug libary from example.
mhightower83 31217f6
Improved build coverage of DEV_DEBUG_ABI_CPP option and operator dele…
mhightower83 ecc7eed
Changed printf format from %S => %s
mhightower83 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't 'new_handler' be independent of debugging?
since the idea is to allow plain new to sometimes avoid throwing (regardless of whether it builds w/ -fexceptions or without). exception-less builds obviously just call abort for would-be exception
ref new_op.cc and new_opnt.cc, nothrow variant just wraps the other in try catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been a while since I did this, I think the increased build size for non-debug builds was a consideration.
It would add 136 bytes of IROM to the non-debug build so that it always builds with our revised
new_handler
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will have to recheck too, then. Debug=enabled without exceptions are not handled though?
Plus there are more missing overloads from -std=c++17
https://en.cppreference.com/w/cpp/memory/new/operator_new
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current master does not support "new" align operations. Compiler reports:
/workdir/repo/gcc-gnu/libstdc++-v3/libsupc++/new_opa.cc:86: undefined reference to
memalign'`No hits on a quick issue check for
memalign
.The library umm_malloc needs an enhancement to support an allocate-aligned option. Also, things will get complicated when incorporating the poison check feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've worked through adding an alignment option to the
umm_malloc
library. In the process, I have addressed an unreported alignment issue.umm_malloc
allocated data addresses always landed on an 8-byte aligned - 4-byte.Now I need to finish working through all the build options in
abi.cpp
andheap.cpp
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(edit: comment thread from above)
Can this be a config error? GCC / newlib / hal / etc.
Reading cppreference and some of the internal GCC stuff from '-faligned-new=X', 'Fundamental alignment' and max_align_t seem to come from 'long double' and 'long long' which are afaik 4-byte ones.
Currently, it does indeed show 8-byte
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...which is sort-of expected, as run-of-the-mill xtensa isa pdf does specify alignment req for long double and long long as 8. compiler here works according to spec, stack and heap alignment would stay consistent vs. current approach
no mention / explicit alignment things mentioned in the machine config, besides the
BIGGEST_ALIGNMENT
of 128 (bits)https://github.com/gcc-mirror/gcc/tree/master/gcc/config/xtensa
/ALIGN/
esp32 variants adhere to a similar standard. however, esp8266 rtos does use 4-byte heap alignment
(which I would assume works just fine for the most part)