[vm/bytecode] Remove alignment of sections in bytecode

Previously, before switching to compact encoding of bytecode
instructions, we had to align bytecode instructions by 4.
Since that time sections of bytecode binary were aligned.

However, this alignment doesn't work well in AOT mode if dill file
contains both bytecode and AST, as other metadata (such as obfuscation
prohibitions) may precede bytecode metadata and may make the whole
bytecode binary misaligned.

This results in incorrect calculation of section offsets,
failed assertion

  assert(writer.offset - start == section.offset);

and VM may fail to load bytecode binary with error

  bytecode_reader.cc: 1368: error: unreachable code

In order to fix these problems, extra alignment of bytecode
sections is removed.

Change-Id: Iaa3b7c46999615aaa446781a7018db409261ead8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/123686
Reviewed-by: Régis Crelier <regis@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>

Author: alexmarkov

pkg/vm/lib/bytecode/bytecode_serialization.dart

@@ -41,7 +41,6 @@ class BufferedWriter {
   final StringWriter stringWriter;
   final ObjectWriter objectWriter;
   final LinkWriter linkWriter;
-  final int baseOffset;
 
   // Prefix of the data stored in this writer.
   // List of pairs Uint8List buffer, int length.
@@ -53,9 +52,8 @@ class BufferedWriter {
   int _currentLength = 0;
   int _nextBufferSize = initialSize << 1;
 
-  BufferedWriter(
-      this.formatVersion, this.stringWriter, this.objectWriter, this.linkWriter,
-      {this.baseOffset: 0});
+  BufferedWriter(this.formatVersion, this.stringWriter, this.objectWriter,
+      this.linkWriter);
 
   factory BufferedWriter.fromWriter(BufferedWriter writer) =>
       new BufferedWriter(writer.formatVersion, writer.stringWriter,
@@ -266,18 +264,6 @@ class BufferedWriter {
     final offset = linkWriter.getOffset(target);
     writePackedUInt30(offset);
   }
-
-  void align(int alignment) {
-    assert(alignment & (alignment - 1) == 0);
-    int offs = baseOffset + offset;
-    int padding = ((offs + alignment - 1) & -alignment) - offs;
-    if (_currentLength > _currentBuffer.length - padding) {
-      _grow();
-    }
-    for (int i = 0; i < padding; ++i) {
-      _addByte(0);
-    }
-  }
 }
 
 class BufferedReader {

pkg/vm/lib/bytecode/declarations.dart

@@ -1365,7 +1365,6 @@ class Component {
     int offset = headerSize;
     for (var section in sections) {
       if (section.writer != null) {
-        offset = (offset + sectionAlignment - 1) & ~(sectionAlignment - 1);
         section.offset = offset;
         offset += section.size;
       } else {
@@ -1384,7 +1383,6 @@ class Component {
     assert(writer.offset - start == headerSize);
     for (var section in sections) {
       if (section.writer != null) {
-        writer.align(sectionAlignment);
         assert(writer.offset - start == section.offset);
         writer.appendWriter(section.writer);
       }

pkg/vm/lib/metadata/bytecode.dart

@@ -53,8 +53,7 @@ class BytecodeMetadataRepository extends MetadataRepository<BytecodeMetadata> {
         bytecodeComponent.version,
         bytecodeComponent.stringTable,
         bytecodeComponent.objectTable,
-        linkWriter,
-        baseOffset: sink.getBufferOffset());
+        linkWriter);
     metadata.write(writer);
     writer.writeContentsToBinarySink(sink);
   }

runtime/tests/vm/dart/bytecode_with_ast_in_aot_test.dart

@@ -0,0 +1,51 @@
+// Copyright (c) 2019, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// OtherResources=bytecode_with_ast_in_aot_test_body1.dart
+// OtherResources=bytecode_with_ast_in_aot_test_body2.dart
+
+// Tests that gen_kernel is able to produce dill file with both bytecode
+// and AST in AOT mode, and gen_snapshot is able to consume them.
+// Two test cases are only different in number of entry points, so
+// obfuscation prohibitions metadata has different size, causing
+// different alignment of bytecode metadata.
+
+import 'dart:io' show Platform;
+
+import 'package:path/path.dart' as path;
+import 'snapshot_test_helper.dart';
+
+testAOTOnBytecodeWithAST(String temp, String source) async {
+  final sourcePath = Platform.script.resolve(source).toFilePath();
+  final dillPath = path.join(temp, '${source}.dill');
+  final snapshotPath = path.join(temp, '${source}.so');
+
+  final genKernelResult = await runGenKernel('BUILD DILL FILE', [
+    '--aot',
+    '--gen-bytecode',
+    '--no-drop-ast',
+    '--output=$dillPath',
+    sourcePath,
+  ]);
+  print(genKernelResult);
+  final genSnapshotResult = await runGenSnapshot('GENERATE SNAPSHOT', [
+    '--use-bytecode-compiler',
+    '--snapshot-kind=app-aot-elf',
+    '--elf=$snapshotPath',
+    dillPath,
+  ]);
+  print(genSnapshotResult);
+  final runResult =
+      await runBinary('RUN SNAPSHOT', dartPrecompiledRuntime, [snapshotPath]);
+  expectOutput("OK", runResult);
+}
+
+main() async {
+  await withTempDir((String temp) async {
+    await testAOTOnBytecodeWithAST(
+        temp, 'bytecode_with_ast_in_aot_test_body1.dart');
+    await testAOTOnBytecodeWithAST(
+        temp, 'bytecode_with_ast_in_aot_test_body2.dart');
+  });
+}

runtime/tests/vm/dart/bytecode_with_ast_in_aot_test_body1.dart

@@ -0,0 +1,19 @@
+// Copyright (c) 2019, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// Test case for bytecode_with_ast_in_aot_test.dart.
+// Contains 1 entry point.
+
+import 'package:expect/expect.dart';
+
+class A {
+  // Make sure obfuscation prohibitions metadata is generated.
+  @pragma('vm:entry-point')
+  void foofoo1() {}
+}
+
+main() {
+  new A();
+  print('OK');
+}

runtime/tests/vm/dart/bytecode_with_ast_in_aot_test_body2.dart

@@ -0,0 +1,24 @@
+// Copyright (c) 2019, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// Test case for bytecode_with_ast_in_aot_test.dart.
+// Contains 2 entry points.
+
+import 'package:expect/expect.dart';
+
+class A {
+  // Make sure obfuscation prohibitions metadata is generated.
+  @pragma('vm:entry-point')
+  void foofoo1() {}
+
+  // One more entry for obfuscation prohibitions metadata,
+  // raises the chances that subsequent bytecode metadata is misaligned.
+  @pragma('vm:entry-point')
+  void foofoo2() {}
+}
+
+main() {
+  new A();
+  print('OK');
+}

runtime/tests/vm/vm.status

@@ -24,6 +24,7 @@ dart/transferable_throws_oom_test: SkipByDesign # This test tries to allocate to
 
 [ $builder_tag == crossword ]
 dart/emit_aot_size_info_flag_test: SkipByDesign # The test itself cannot determine the location of gen_snapshot (only tools/test.py knows where it is).
+dart/bytecode_with_ast_in_aot_test: SkipByDesign # The test doesn't know location of cross-platform gen_snapshot
 
 [ $builder_tag == obfuscated ]
 dart/causal_stacks/*: SkipByDesign # Asserts exact stacktrace output.
@@ -244,6 +245,7 @@ dart/redirection_type_shuffling_test/none: RuntimeError
 [ $mode == debug || $runtime != dart_precompiled || $system == android ]
 dart/emit_aot_size_info_flag_test: SkipByDesign # This test is for VM AOT only and is quite slow (so we don't run it in debug mode).
 dart/use_bare_instructions_flag_test: SkipByDesign # This test is for VM AOT only and is quite slow (so we don't run it in debug mode).
+dart/bytecode_with_ast_in_aot_test: SkipByDesign # This test is for VM AOT only and is quite slow (so we don't run it in debug mode).
 
 # It makes no sense to run any test that uses spawnURI under the simulator
 # as that would involve running CFE (the front end) in simulator mode