Fix PAL double initialize #2296
Conversation
obastemur
force-pushed
the
pal_fix_double_init
branch
2 times, most recently
from
9d5640d to
5e9ca6a
Compare
|
Second part of this work requires #2288 is merged. |
| memcpy(path, msg, (size_t)str_len); \ | ||
| path[str_len] = char(0) | ||
|
|
||
| void GetBinaryLocation(char *path, const unsigned size) |
There was a problem hiding this comment.
size [](start = 50, length = 4)
Specify type for size explicitly?
There was a problem hiding this comment.
We had discussed on this when it was implemented for the first time. It should be reachable from the blame part.
obastemur
force-pushed
the
pal_fix_double_init
branch
from
5e9ca6a to
d55cd22
Compare
|
Adding additional [ Android / ARMv7 / iOS ] related commits straight to this PR. Fixes are related. A full review on this PR is highly appreciated. |
|
moved custom |
…ptimizing the symbols away
- Automated preq. installation for Debian / RedHat distros Build/compile_clang.sh This script downloads and compiles llvm/clang/lto-gold projects [3.9.1]
obastemur
force-pushed
the
pal_fix_double_init
branch
from
9be3130 to
b33e26f
Compare
|
|
||
| LLVM_VERSION="3.9.1" | ||
|
|
||
| CC_URL="git://sourceware.org/git/binutils-gdb.git\nhttp://llvm.org/releases/${LLVM_VERSION}/\n" |
|
You can now resolve the CI issues as described here: #2332 (comment) |
obastemur
force-pushed
the
pal_fix_double_init
branch
from
b33e26f to
ef987bb
Compare
|
"Fix PAL double initialize" -- Could you document in comment or commit message in more details, how previously PAL double initializes and now does not? From the surface it looks we have 3+ places calling PAL_InitializeChakraCore(). |
|
|
||
| __attribute__((no_instrument_function)) | ||
| static void | ||
| GetBinaryLocation() |
There was a problem hiding this comment.
Intentional. See comments, this file shouldn't take dependencies
There was a problem hiding this comment.
Yes, I was aware of that. Questionable though if it is worth duplicating code for that goal.
Also, is this only useful for diagnostics builds and can be excluded form "release"?
There was a problem hiding this comment.
It is already excluded for any configuration with no --trace. -> #ifdef ENABLE_CC_XPLAT_TRACE
| // All args are right after ReturnAddress by custom calling convention | ||
| Js::Var* pArgs = reinterpret_cast<Js::Var*>(addrOfReturnAddress) + 1; | ||
| #ifdef _ARM_ | ||
| n += 2; // ip + fp |
There was a problem hiding this comment.
Could you please document more details for this "+ 2"? Android ARM stores "ip + fp" in between return address and args?
There was a problem hiding this comment.
Yes. Not just Android ARM, so ifdef _ARM_. Still work in progress here though.
Previously we were calling 2 sep. methods and those were ending up calling the same final initializer multiple times. ATM, we have multiple entry points (Shared / CH / Static) and we trigger base initializer only once. |
|
@ThomsonTan @jianchun Thanks for the review |
Merge pull request #2296 from obastemur:pal_fix_double_init
6 participants
No description provided.