Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(22)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2934263002: Fix to 32-bit kernel. (Closed)

Created:
3 years, 6 months ago by jensj
Modified:
3 years, 6 months ago
CC:
reviews_dartlang.org, vm-dev_dartlang.org
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -1 line) Patch
M runtime/vm/kernel_binary_flowgraph.cc View 1 chunk +3 lines, -1 line 0 comments Download

Messages

Total messages: 7 (2 generated)
jensj
3 years, 6 months ago (2017-06-14 13:31:05 UTC) #2
Vyacheslav Egorov (Google)
LGTM we might want to search through the whole flow graph builder looking for the ...
3 years, 6 months ago (2017-06-14 13:33:28 UTC) #3
Paul Berry
lgtm This fixes the crashes I was experiencing, thanks
3 years, 6 months ago (2017-06-14 13:48:54 UTC) #4
jensj
Committed patchset #1 (id:1) manually as 840b3700b735ff23664721f18f229b6e82a8bdc7 (presubmit successful).
3 years, 6 months ago (2017-06-14 13:50:19 UTC) #6
jensj
3 years, 6 months ago (2017-06-14 13:55:30 UTC) #7
Message was sent while issue was closed. 
I went through the remaining cases of "fragment + fragment" (see diff below,
though I don't suggest we do that) --- and the rest seems fine to me...

```
diff --git a/runtime/vm/kernel_binary_flowgraph.cc
b/runtime/vm/kernel_binary_flowgraph.cc
index 33c85883c0..e64daa3cdb 100644
--- a/runtime/vm/kernel_binary_flowgraph.cc
+++ b/runtime/vm/kernel_binary_flowgraph.cc
@@ -3430,3 +3430,3 @@ FlowGraph*
StreamingFlowGraphBuilder::BuildGraphOfFunction(
     // TODO(29737): This sequence should be generated in order.
-    body = instructions + body;
+    body = instructions ^ body;
     flow_graph_builder_->context_depth_ = current_context_depth;
@@ -3459,3 +3459,3 @@ FlowGraph*
StreamingFlowGraphBuilder::BuildGraphOfFunction(
     // TODO(29737): This sequence should be generated in order.
-    body = DebugStepCheck(check_pos) + body;
+    body = DebugStepCheck(check_pos) ^ body;
     flow_graph_builder_->context_depth_ = current_context_depth;
@@ -4851,3 +4851,3 @@ Fragment
StreamingFlowGraphBuilder::BuildVariableSet(TokenPosition* p) {
   if (NeedsDebugStepCheck(stack(), position)) {
-    instructions = DebugStepCheck(position) + instructions;
+    instructions = DebugStepCheck(position) ^ instructions;
   }
@@ -4868,3 +4868,3 @@ Fragment
StreamingFlowGraphBuilder::BuildVariableSet(uint8_t payload,
   if (NeedsDebugStepCheck(stack(), position)) {
-    instructions = DebugStepCheck(position) + instructions;
+    instructions = DebugStepCheck(position) ^ instructions;
   }
@@ -4887,3 +4887,3 @@ Fragment
StreamingFlowGraphBuilder::BuildPropertyGet(TokenPosition* p) {
 
-  return instructions + InstanceCall(position, getter_name, Token::kGET, 1);
+  return instructions ^ InstanceCall(position, getter_name, Token::kGET, 1);
 }
@@ -4909,3 +4909,3 @@ Fragment
StreamingFlowGraphBuilder::BuildPropertySet(TokenPosition* p) {
   instructions += InstanceCall(position, setter_name, Token::kSET, 2);
-  return instructions + Drop();
+  return instructions ^ Drop();
 }
@@ -4941,3 +4941,3 @@ Fragment
StreamingFlowGraphBuilder::BuildDirectPropertyGet(TokenPosition* p) {
   instructions += PushArgument();
-  return instructions + StaticCall(position, target, 1);
+  return instructions ^ StaticCall(position, target, 1);
 }
@@ -4967,3 +4967,3 @@ Fragment
StreamingFlowGraphBuilder::BuildDirectPropertySet(TokenPosition* p) {
 
-  return instructions + Drop();
+  return instructions ^ Drop();
 }
@@ -4990,3 +4990,3 @@ Fragment
StreamingFlowGraphBuilder::BuildStaticGet(TokenPosition* p) {
         Fragment instructions = Constant(field);
-        return instructions + LoadStaticField();
+        return instructions ^ LoadStaticField();
       } else {
@@ -5023,3 +5023,3 @@ Fragment
StreamingFlowGraphBuilder::BuildStaticSet(TokenPosition* p) {
     if (NeedsDebugStepCheck(stack(), position)) {
-      instructions = DebugStepCheck(position) + instructions;
+      instructions = DebugStepCheck(position) ^ instructions;
     }
@@ -5029,3 +5029,3 @@ Fragment
StreamingFlowGraphBuilder::BuildStaticSet(TokenPosition* p) {
     instructions += LoadLocal(variable);
-    return instructions + StoreStaticField(position, field);
+    return instructions ^ StoreStaticField(position, field);
   } else {
@@ -5047,3 +5047,3 @@ Fragment
StreamingFlowGraphBuilder::BuildStaticSet(TokenPosition* p) {
     // Drop the unused result & leave the stored value on the stack.
-    return instructions + Drop();
+    return instructions ^ Drop();
   }
@@ -5109,3 +5109,3 @@ Fragment
StreamingFlowGraphBuilder::BuildMethodInvocation(TokenPosition* p) {
         token_kind == Token::kEQ ? Token::kEQ_STRICT : Token::kNE_STRICT;
-    return instructions +
+    return instructions ^
            StrictCompare(strict_cmp_kind, /*number_check = */ true);
@@ -5168,3 +5168,3 @@ Fragment
StreamingFlowGraphBuilder::BuildDirectMethodInvocation(
         token_kind == Token::kEQ ? Token::kEQ_STRICT : Token::kNE_STRICT;
-    return instructions +
+    return instructions ^
            StrictCompare(strict_cmp_kind, /*number_check = */ true);
@@ -5182,3 +5182,3 @@ Fragment
StreamingFlowGraphBuilder::BuildDirectMethodInvocation(
   ++argument_count;
-  return instructions + StaticCall(TokenPosition::kNoSource, target,
+  return instructions ^ StaticCall(TokenPosition::kNoSource, target,
                                    argument_count, argument_names);
@@ -5359,3 +5359,3 @@ Fragment
StreamingFlowGraphBuilder::BuildConstructorInvocation(
   instructions += StaticCall(position, target, argument_count, argument_names);
-  return instructions + Drop();
+  return instructions ^ Drop();
 }
@@ -5411,3 +5411,4 @@ Fragment
StreamingFlowGraphBuilder::BuildLogicalExpression(
 
-  return Fragment(instructions.entry, join) +
+  // Safe because the first one is side-effect-free, right?
+  return Fragment(instructions.entry, join) ^
          LoadLocal(parsed_function()->expression_temp_var());
@@ -5447,3 +5448,4 @@ Fragment
StreamingFlowGraphBuilder::BuildConditionalExpression(
 
-  return Fragment(instructions.entry, join) +
+  // Safe because the first one is side-effect-free, right?
+  return Fragment(instructions.entry, join) ^
          LoadLocal(parsed_function()->expression_temp_var());
@@ -5658,3 +5660,3 @@ Fragment
StreamingFlowGraphBuilder::BuildThrow(TokenPosition* p) {
   if (NeedsDebugStepCheck(stack(), position)) {
-    instructions = DebugStepCheck(position) + instructions;
+    instructions = DebugStepCheck(position) ^ instructions;
   }
@@ -5713,3 +5715,3 @@ Fragment StreamingFlowGraphBuilder::BuildListLiteral(bool
is_const,
 
-  return instructions + StaticCall(position, factory_method, 2);
+  return instructions ^ StaticCall(position, factory_method, 2);
 }
@@ -5774,3 +5776,3 @@ Fragment StreamingFlowGraphBuilder::BuildMapLiteral(bool
is_const,
 
-  return instructions + StaticCall(position, factory_method, 2);
+  return instructions ^ StaticCall(position, factory_method, 2);
 }
@@ -6317,3 +6319,3 @@ Fragment StreamingFlowGraphBuilder::BuildSwitchStatement()
{
         body_join = block.DestinationDirect(i);
-        body_fragments[i] = Fragment(body_join) + body_fragments[i];
+        body_fragments[i] = Fragment(body_join) ^ body_fragments[i];
       }
@@ -6559,3 +6561,3 @@ Fragment StreamingFlowGraphBuilder::BuildTryCatch() {
 
-        Fragment(catch_entry) + catch_handler_body;
+        Fragment(catch_entry) ^ catch_handler_body;
         catch_body = Fragment(next_catch_entry);
@@ -6782,3 +6784,3 @@ Fragment
StreamingFlowGraphBuilder::BuildVariableDeclaration() {
   if (NeedsDebugStepCheck(stack(), debug_position)) {
-    instructions = DebugStepCheck(debug_position) + instructions;
+    instructions = DebugStepCheck(debug_position) ^ instructions;
   }
diff --git a/runtime/vm/kernel_to_il.cc b/runtime/vm/kernel_to_il.cc
index f9f4e35119..e5172d357c 100644
--- a/runtime/vm/kernel_to_il.cc
+++ b/runtime/vm/kernel_to_il.cc
@@ -61,3 +61,3 @@ Fragment Fragment::closed() {
 
-Fragment operator+(const Fragment& first, const Fragment& second) {
+Fragment operator^(const Fragment& first, const Fragment& second) {
   Fragment result = first;
@@ -997,3 +997,3 @@ Fragment FlowGraphBuilder::BranchIfTrue(TargetEntryInstr**
then_entry,
   Fragment instructions = Constant(Bool::True());
-  return instructions + BranchIfEqual(then_entry, otherwise_entry, negate);
+  return instructions ^ BranchIfEqual(then_entry, otherwise_entry, negate);
 }
@@ -1005,3 +1005,3 @@ Fragment FlowGraphBuilder::BranchIfNull(TargetEntryInstr**
then_entry,
   Fragment instructions = NullConstant();
-  return instructions + BranchIfEqual(then_entry, otherwise_entry, negate);
+  return instructions ^ BranchIfEqual(then_entry, otherwise_entry, negate);
 }
@@ -1954,3 +1954,3 @@ Fragment FlowGraphBuilder::NativeFunctionBody(intptr_t
first_positional_offset,
   }
-  return body + Return(TokenPosition::kNoSource);
+  return body ^ Return(TokenPosition::kNoSource);
 }
diff --git a/runtime/vm/kernel_to_il.h b/runtime/vm/kernel_to_il.h
index 959197744a..80afb369a2 100644
--- a/runtime/vm/kernel_to_il.h
+++ b/runtime/vm/kernel_to_il.h
@@ -188,3 +188,3 @@ class Fragment {
 
-Fragment operator+(const Fragment& first, const Fragment& second);
+Fragment operator^(const Fragment& first, const Fragment& second);
 Fragment operator<<(const Fragment& fragment, Instruction* next);
```

Powered by Google App Engine
This is Rietveld 408576698