Update debug information for ppc64 #2
Labels
Comments
|
check |
lbianc
pushed a commit
that referenced
this issue
Sep 1, 2015
Summary: In a switch statement with a fallthrough we would use the same environemnt
for the next case we encounter. However this is incorrect. Consider the
following case.
```
$x = 0; // int
switch (0) {
case 1:
$x = '';
// FALLTHROUGH
case 2:
$x; // <-- #1
}
$x; // <-- #2
```
Currently at position #1 we will think $x is a string, since that is what we
assigned $x to be in the above case. However we could have switched directly
into case 2 without a fallthrough at which point $x would be an int.
This bug also impacts the result outside the switch statement. Because we
believe $x is of type string in both branches we believe $x is a string at
position #2 as well.
The solution is to intersect the local environment before and after type
checking the block in case 1. This intersected local environment is what should
be passed as the environment for case 2.
Reviewed By: @jwatzman, @int3
Differential Revision: D2230208
gut
pushed a commit
that referenced
this issue
Sep 22, 2015
Summary: Flow is: 1. async curl request starts 2. call to scheduleTimeout is queued in main thread 3. async curl request finishes 4. timeout is cleared... but the scheduleTimeout() in #2 might not have happened if the http response was really, really, fast Backtrace of crashes showed `~CurlMultiAwait` and `scheduleTiemout` in multiple threads. Reviewed By: @JoelMarcey Differential Revision: D2443938
lbianc
pushed a commit
that referenced
this issue
Jun 1, 2016
Summary: Diff #2 of 14. Reviewed By: mzlee Differential Revision: D3115972 fbshipit-source-id: c75cbc95f37e8f2c2bf81311a8ca08ee766fc107
|
I guess this task needs to be updated or closed since i guess all this tasks are done. |
|
as far as I remember, the first point is still open, but unfortunately nobody is working on it. If it's the case, then also the 'dwarf' branch can be closed. |
|
@gut why dwarf branch cannot be merged with next? |
|
it's only partly implemented. |
|
@gut since you work with this problem before can you fill this task with more information? Why this is need for? What's the test case for this? With more information about it someone else can work on this task if that's the case. |
|
I worked with this 1 year ago. I can't really say I remember a lot. First thing we have to check is if we'll have any gain in implementing this. |
|
Well, this task does not seem relevant anymore. We were debugging this code since then and found no reason to implement this... I'll close this and remove the branch. Just to keep track, the branch had this top: https://github.com/PPC64/hhvm/tree/6ff4f7bf6193559b452b44ecb17f38a6860ddcb1 |
racardoso
pushed a commit
that referenced
this issue
Mar 30, 2017
Summary:
This diff models typing for optional shape fields.
= Model
These fields are modeled as follows:
```name=typing_defs.ml,lang=ocaml
'phase shape_field_type = {
sft_optional : bool;
sft_ty : 'phase ty;
}
```
And integrated into the existing type structure as follows:
```name=typing_defs.ml,lang=diff
ty_ = (* ...existing types... *)
| Tshape :
shape_fields_known *
- ('phase ty Nast.ShapeMap.t)
+ ('phase shape_field_type Nast.ShapeMap.t)
-> 'phase ty_
```
= Justification
There were two reasonable ways of modeling optional shape fields:
# `Tshape` remains unchanged, and a new toplevel type `Toptional_shape_field` is introduced.
# `Tshape` is changed to hold `shape_field_type`s that explicitly denote whether they are optional or not.
I have opted for #2.
Choice #1 has these trade-offs:
# (good) The shape handling code remains unchanged.
# (ok/good) `Tshape` is symmetric in a sense, and does not need additional handling logic. Handling logic can be placed closer to where toplevel processing for `ty` appears.
# (bad) **Everywhere** that a `ty` is processed, a new case has to be added to handle the optional shape field. In most of these cases, optional shape fields are not even logically possible!
# (extremely bad) There is special casing logic that needs to be written for optional shape fields, and this logic must be done **in the context of a `Tshape`**. This design would break the abstraction, forcing us to push through `Tshape` information to the toplevel processing for `ty`.
Choice #2 has these trade-offs:
# (good) An `shape_field_type` may //only// appear in the context of a `Tshape`. We will never process an `shape_field_type` in a place where it's not logically possible. Consequently, the processing always has visibility to the `Tshape` of which the `shape_field_type` is a part of.
# (good) This design forces the developer to handle processing for `shape_field_type`s when processing a `Tshape`. It will be very hard to "forget" to handle this case, since it will be enforced by the type system.
# (bad) It turns out there are lots of places that don't care about whether a type is optional or not. These points in the code now have to be filled with boilerplate to unwrap the `ty` from the `shape_field_type`.
= Conclusion
Whereas #1 has serious logic and maintainability concerns, #2 really just has concerns related to code cleanliness. Additionally, to handle those code cleanliness concerns, I created the `typing_helpers` library to handle patterns that occurred repeatedly in this diff.
= Other notes
There are lots of locations in this diff where I'm not completely sure what's going on. Please let me know if there are any areas that look suspect. If things do in fact look off, it might be useful for us to go through some parts of the diff in person. Since the existing typecheck tests and the new ones for the optional shape fields all pass, I'm reasonably confident that there's not a regression, but I can't be sure.
Reviewed By: dlreeves
Differential Revision: D4563246
fbshipit-source-id: da8d446429351bf804c0485335c29ab83fd049da
racardoso
pushed a commit
that referenced
this issue
Apr 27, 2017
…generated Summary: Generalizes `coroutine.ml` to support modes, and implements two new modes, one of which is used for testing. There are now three modes: # (Default): Print the lowered syntax tree's text. # `--reparse-compare`: Compare the lowered syntax tree and the syntax tree reparsed from the lowered source text. # `--syntax-tree`: Print the lowered syntax tree. More details on the `--reparse-compare` mode: The full fidelity parser is optimized for consuming text and producing syntax. It is not optimized for building up syntax from nothing. It's tricky to get all of the cases right. While generating code, some questions come to mind: 1) Does this single element actually belong in a `SyntaxList`? 2) Should this element be wrapped in a `list_item`? 3) Should this name be wrapped in a qualified name syntax? Et cetera. So, I extended the test. It now does this: 1) Lower coroutines from the parse tree. Call the result of this operation the `original_syntax_tree`. 2) Spit out a textual representation of the `original_syntax_tree`. 3) Re-parse the textual representation of the `original_syntax_tree` using the full-fidelity parser. No lowering business this time. Call the result of this operation the `reparsed_syntax_tree`. 4) Spit out a textual representation of the `reparsed_syntax_tree`. 5) Compare the results from #2 and #4. If they're different, we have a problem! That would mean that the full-fidelity parser created a //different// `SyntaxTree` for the //same// source text, and therefore we've code-generated our `SyntaxTree` incorrectly. **Note:** This //still// does not typecheck the lowered result. I'll do that in another diff soon. Reviewed By: ericlippert Differential Revision: D4919865 fbshipit-source-id: d2725e0fd50997022f0363975b1e828e414d5464
racardoso
pushed a commit
that referenced
this issue
Apr 27, 2017
Summary: Fixes two issues that existed previously: 1. We want the coroutine's enclosing class's name to be part of the generated state machine's name, since multiple classes may have coroutines with the same name. I've also replaced the term `StateMachine` with `Closure`. 2. Previously, I was simply taking the `text` of the `function_name` node. This is not good, as it includes trivia. Instead, I now `syntax` the `function_name` node in order to get the underlying `Token`. From the `Token`, it is easy to get the name without trivia. This also applies to the classname. Fixing #2 correctly required a bit of reworking. I wanted to avoid re-`syntax`ing the classname multiple times (and it should also be done as early as possible to short circuit in the case of unexpected input). Consequently, I pulled it all the way up into when we first `syntax` the `ClassishDeclaration`, and then threaded it down through everything else. I used a similar approach for the `function_name`, and factored out a little bit of duplicated logic from `coroutine_method_lowerer.ml` and `coroutine_state_machine_generator.ml` into `coroutine_lowerer.ml`. This "threading through" could get unwieldy in the future if we need to thread more things through, so I'm very open to suggestions on how to improve this. IMO, it's still pretty manageable, so it's probably OK to punt for now. Reviewed By: ericlippert Differential Revision: D4931102 fbshipit-source-id: dae7b45c80a7dce5e577b8ffdb86448848f019a8
lbianc
pushed a commit
that referenced
this issue
Jul 4, 2017
Summary:
This creates an ARMv8 specific Vinstr to represent a commonly used form of the
ubfm/ubfx instruction and the peephole to detect it. This pattern was seen
in several of the regression tests. It is generated during the lowering of the callm
instruction.
Before
=====
0x434009c4 53001c21 uxtb w1, w1
0x434009c8 53027c21 lsr w1, w1, #2
0x434009cc d37df021 lsl x1, x1, #3
0x434009d0 d297c302 movz x2, #0xbe18
0x434009d4 f2a1c322 movk x2, #0xe19, lsl #16
0x434009d8 f8626821 ldr x1, [x1, x2]
0x434009dc d63f0020 blr x1
After
=====
0x40400994 53021c21 ubfx w1, w1, #2, #6
0x40400998 d37df021 lsl x1, x1, #3
0x4040099c d2965e02 movz x2, #0xb2f0
0x404009a0 f2a081a2 movk x2, #0x40d, lsl #16
0x404009a4 f8626821 ldr x1, [x1, x2]
0x404009a8 d63f0020 blr x1
This was seen 169 times in hphp/test/quick/all_type_comparison_test.php and
230 times in hphp/test/zend/good/ext/intl/tests/grapheme.php.
The standard regression tests were run with 6 option sets. No new regressions seen.
Closes facebook#7820
Differential Revision: D5063904
Pulled By: mxw
fbshipit-source-id: 7a2fd19e498b09e8d2a01c6bb7abc575aa184727
lbianc
pushed a commit
that referenced
this issue
Sep 26, 2017
Summary: Reported by UBSAN: ```lang=bash 001+ hphp/runtime/debugger/debugger_client.cpp:2515:7: runtime error: load of value 190, which is not a valid value for type 'bool' 002+ #0 0xa04d7ed in HPHP::Eval::DebuggerClient::saveConfig() hphp/runtime/debugger/debugger_client.cpp:2515 003+ #1 0xa02b046 in HPHP::Eval::DebuggerClient::setDebuggerClientSmallStep(bool const&) hphp/runtime/debugger/debugger_client.h:352 004+ #2 0xa02b046 in HPHP::Eval::DebuggerClient::loadConfig() hphp/runtime/debugger/debugger_client.cpp:2391 005+ #3 0xa02995f in HPHP::Eval::DebuggerClient::init(HPHP::Eval::DebuggerClientOptions const&) hphp/runtime/debugger/debugger_client.cpp:750 006+ #4 0xa015d01 in HPHP::Eval::DebuggerClient::start(HPHP::Eval::DebuggerClientOptions const&) hphp/runtime/debugger/debugger_client.cpp:783 007+ #5 0xa015d01 in HPHP::Eval::DebuggerClient::Start(HPHP::Eval::DebuggerClientOptions const&) hphp/runtime/debugger/debugger_client.cpp:399 008+ #6 0x9fe833b in HPHP::Eval::Debugger::StartClient(HPHP::Eval::DebuggerClientOptions const&) hphp/runtime/debugger/debugger.cpp:56 009+ #7 0x884b41f in HPHP::execute_program_impl(int, char**) hphp/runtime/base/program-functions.cpp:1929 010+ #8 0x883d586 in HPHP::execute_program(int, char**) hphp/runtime/base/program-functions.cpp:1200 011+ #9 0x72782e in main hphp/hhvm/main.cpp:85 012+ #10 0x7ffbeed1c857 in __libc_start_main /home/engshare/third-party2/glibc/2.23/src/glibc-2.23/csu/../csu/libc-start.c:289 013+ #11 0x724e28 in _start /home/engshare/third-party2/glibc/2.23/src/glibc-2.23/csu/../sysdeps/x86_64/start.S:118 015+ SUMMARY: UndefinedBehaviorSanitizer: invalid-bool-load hphp/runtime/debugger/debugger_client.cpp:2515:7 in ``` Reviewed By: markw65 Differential Revision: D5898634 fbshipit-source-id: 102f90ead9766b381a4dc722800ad6e7fb8a76be
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
hphp/runtime/vm/debug/files for correct code position information on ppc64 emitted codehphp/runtime/vm/jit/unwind-x64.cppor create a new unwind file for ppc64The text was updated successfully, but these errors were encountered: