fix: Incorrect module layout generated when data definition operands have different alignments

clients/vscode-hlasmplugin/CHANGELOG.md

@@ -8,7 +8,7 @@
 
 #### Fixed
 - Highlighting now fully works with themes, not just categories dark, light and contrast.
-
+- Incorrect module layout generated when data defintion operands have different alignments
 
 ## [0.15.1](https://github.com/eclipse/che-che4z-lsp-for-hlasm/compare/0.15.0...0.15.1) (2021-11-11)
 

parser_library/src/processing/instruction_sets/CMakeLists.txt

@@ -15,6 +15,7 @@ target_sources(parser_library PRIVATE
 	asm_processor.h
 	ca_processor.cpp
 	ca_processor.h
+	data_def_postponed_statement.cpp
 	data_def_postponed_statement.h
 	instruction_processor.h
 	low_language_processor.cpp

parser_library/src/processing/instruction_sets/asm_processor.cpp

@@ -213,23 +213,6 @@ void asm_processor::process_data_instruction(rebuilt_statement stmt)
     context::address loctr = hlasm_ctx.ord_ctx.align(al);
     context::ordinary_assembly_dependency_solver dep_solver(hlasm_ctx.ord_ctx, loctr);
 
-    // dependency sources is list of all expressions in data def operand, that have some unresolved dependencies.
-    bool has_dependencies = false;
-    // has_length_dependencies specifies whether the length of the data instruction can be resolved right now or must be
-    // postponed
-    bool has_length_dependencies = false;
-    for (const auto& op : stmt.operands_ref().value)
-    {
-        auto data_op = op->access_data_def();
-
-        has_dependencies |= data_op->has_dependencies(dep_solver);
-
-        has_length_dependencies |= data_op->get_length_dependencies(dep_solver).contains_dependencies();
-
-        // some types require operands that consist only of one symbol
-        data_op->value->check_single_symbol_ok(diagnostic_collector(this));
-    }
-
     // process label
     auto label = find_label_symbol(stmt);
 
@@ -267,55 +250,112 @@ void asm_processor::process_data_instruction(rebuilt_statement stmt)
             add_diagnostic(diagnostic_op::error_E031("symbol", stmt.label_ref().field_range));
     }
 
+    const auto& operands = stmt.operands_ref().value;
+
+    const context::resolvable* l_dep = nullptr;
+    const context::resolvable* s_dep = nullptr;
+    if (label != context::id_storage::empty_id)
+    {
+        auto data_op = operands.front()->access_data_def();
+
+        if (data_op->value->length && data_op->value->length->get_dependencies(dep_solver).contains_dependencies())
+            l_dep = data_op->value->length.get();
+
+        if (data_op->value->scale && data_op->value->scale->get_dependencies(dep_solver).contains_dependencies())
+            s_dep = data_op->value->scale.get();
+    }
+
     // TODO issue warning when alignment is bigger than section's alignment
     // hlasm_ctx.ord_ctx.current_section()->current_location_counter().
 
+    // dependency sources is list of all expressions in data def operand, that have some unresolved dependencies.
+    bool has_dependencies = false;
 
-    if (has_dependencies)
+    std::vector<data_def_dependency<instr_type>> dependencies;
+    std::vector<context::space_ptr> dependencies_spaces;
+
+    // Why is this so complicated?
+    // 1. We cannot represent the individual operands because of bitfields.
+    // 2. We cannot represent the whole area as a single dependency when the alignment requirements are growing.
+    // Therefore, we split the operands into chunks depending on the alignent.
+    // Whenever the alignment requirement increases between consecutive operands, we start a new chunk.
+    for (auto it = operands.begin(); it != operands.end();)
     {
-        auto adder = hlasm_ctx.ord_ctx.symbol_dependencies.add_dependencies(
-            std::make_unique<data_def_postponed_statement<instr_type>>(std::move(stmt), hlasm_ctx.processing_stack()),
-            dep_solver.derive_current_dependency_evaluation_context());
-        if (has_length_dependencies)
-        {
-            auto sp = hlasm_ctx.ord_ctx.register_ordinary_space(al);
-            adder.add_dependency(
-                sp, dynamic_cast<const data_def_postponed_statement<instr_type>*>(&*adder.source_stmt));
-        }
-        else
-            hlasm_ctx.ord_ctx.reserve_storage_area(
-                data_def_postponed_statement<instr_type>::get_operands_length(
-                    adder.source_stmt->resolved_stmt()->operands_ref().value, dep_solver),
-                context::no_align);
+        const auto start = it;
 
-        bool cycle_ok = true;
+        const auto initial_alignment = (*it)->access_data_def()->value->get_alignment();
+        context::address op_loctr = hlasm_ctx.ord_ctx.align(initial_alignment);
+        data_def_dependency_solver op_solver(dep_solver, &op_loctr);
+
+        auto current_alignment = initial_alignment;
+
+        // has_length_dependencies specifies whether the length of the data instruction can be resolved right now or
+        // must be postponed
+        bool has_length_dependencies = false;
 
-        if (label != context::id_storage::empty_id)
+        for (; it != operands.end(); ++it)
         {
-            auto data_op = adder.source_stmt->resolved_stmt()->operands_ref().value.front()->access_data_def();
+            const auto& op = *it;
+
+            auto data_op = op->access_data_def();
+            auto op_align = data_op->value->get_alignment();
+
+            // leave for the next round to make sure that the actual alignment is computed correctly
+            if (op_align.boundary > current_alignment.boundary)
+                break;
+            current_alignment = op_align;
+
+            has_dependencies |= data_op->has_dependencies(op_solver);
 
-            if (data_op->value->length && data_op->value->length->get_dependencies(dep_solver).contains_dependencies())
-                cycle_ok = adder.add_dependency(label, context::data_attr_kind::L, data_op->value->length.get());
+            has_length_dependencies |= data_op->get_length_dependencies(op_solver).contains_dependencies();
 
-            if (data_op->value->scale && data_op->value->scale->get_dependencies(dep_solver).contains_dependencies())
-                cycle_ok &= adder.add_dependency(label, context::data_attr_kind::S, data_op->value->scale.get());
+            // some types require operands that consist only of one symbol
+            (void)data_op->value->check_single_symbol_ok(diagnostic_collector(this));
+        }
+
+        const auto* b = &*start;
+        const auto* e = b + (it - start);
+
+        if (has_length_dependencies)
+        {
+            dependencies.emplace_back(b, e, std::move(op_loctr));
+            dependencies_spaces.emplace_back(hlasm_ctx.ord_ctx.register_ordinary_space(current_alignment));
+        }
+        else
+        {
+            auto length = data_def_dependency<instr_type>::get_operands_length(b, e, op_solver);
+            hlasm_ctx.ord_ctx.reserve_storage_area(length, context::no_align);
         }
+    }
 
+    if (has_dependencies)
+    {
+        auto dep_stmt = std::make_unique<data_def_postponed_statement<instr_type>>(
+            std::move(stmt), hlasm_ctx.processing_stack(), std::move(dependencies));
+        const auto& deps = dep_stmt->get_dependencies();
+
+        auto adder = hlasm_ctx.ord_ctx.symbol_dependencies.add_dependencies(
+            std::move(dep_stmt), dep_solver.derive_current_dependency_evaluation_context());
         adder.add_dependency();
 
+        bool cycle_ok = true;
+
+        if (l_dep)
+            cycle_ok &= adder.add_dependency(label, context::data_attr_kind::L, l_dep);
+        if (s_dep)
+            cycle_ok &= adder.add_dependency(label, context::data_attr_kind::S, s_dep);
+
         if (!cycle_ok)
-            add_diagnostic(diagnostic_op::error_E033(
-                adder.source_stmt->resolved_stmt()->operands_ref().value.front()->operand_range));
+            add_diagnostic(diagnostic_op::error_E033(operands.front()->operand_range));
+
+        auto sp = dependencies_spaces.begin();
+        for (const auto& d : deps)
+            adder.add_dependency(std::move(*sp++), &d);
 
         adder.finish();
     }
     else
-    {
-        hlasm_ctx.ord_ctx.reserve_storage_area(
-            data_def_postponed_statement<instr_type>::get_operands_length(stmt.operands_ref().value, dep_solver),
-            context::no_align);
         check(stmt, hlasm_ctx.processing_stack(), dep_solver, checker_, *this);
-    }
 }
 
 void asm_processor::process_DC(rebuilt_statement stmt)

parser_library/src/processing/instruction_sets/data_def_postponed_statement.cpp

@@ -0,0 +1,129 @@
+/*
+ * Copyright (c) 2021 Broadcom.
+ * The term "Broadcom" refers to Broadcom Inc. and/or its subsidiaries.
+ *
+ * This program and the accompanying materials are made
+ * available under the terms of the Eclipse Public License 2.0
+ * which is available at https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *
+ * Contributors:
+ *   Broadcom, Inc. - initial API and implementation
+ */
+
+#include "data_def_postponed_statement.h"
+
+#include <limits>
+
+#include "semantics/operand_impls.h"
+
+namespace hlasm_plugin::parser_library::processing {
+
+template<checking::data_instr_type instr_type>
+data_def_postponed_statement<instr_type>::data_def_postponed_statement(rebuilt_statement stmt,
+    context::processing_stack_t stmt_location_stack,
+    std::vector<data_def_dependency<instr_type>> dependencies)
+    : postponed_statement_impl(std::move(stmt), std::move(stmt_location_stack))
+    , m_dependencies(std::move(dependencies))
+{}
+
+// Inherited via resolvable
+template<checking::data_instr_type instr_type>
+context::dependency_collector data_def_dependency<instr_type>::get_dependencies(
+    context::dependency_solver& _solver) const
+{
+    data_def_dependency_solver solver(_solver, &m_loctr);
+    context::dependency_collector conjunction;
+    for (auto it = m_begin; it != m_end; ++it)
+    {
+        const auto& op = *it;
+        if (op->type == semantics::operand_type::EMPTY)
+            continue;
+        conjunction = conjunction + op->access_data_def()->get_length_dependencies(solver);
+    }
+    return conjunction;
+}
+
+template<checking::data_instr_type instr_type>
+int32_t data_def_dependency<instr_type>::get_operands_length(const semantics::operand_ptr* b,
+    const semantics::operand_ptr* e,
+    context::dependency_solver& _solver,
+    const context::address* loctr)
+{
+    data_def_dependency_solver solver(_solver, loctr);
+
+    constexpr const auto round_up_bytes = [](uint64_t& v, uint64_t bytes) { v = checking::round_up(v, bytes * 8); };
+
+    for (auto it = b; it != e; ++it)
+    {
+        const auto& op = *it;
+        if (op->type == semantics::operand_type::EMPTY)
+            continue;
+
+        if (auto dd = op->access_data_def()->value.get();
+            dd->length_type != expressions::data_definition::length_type::BIT)
+        {
+            // align to whole byte
+            round_up_bytes(solver.operands_bit_length, 1);
+
+            // enforce data def alignment
+            round_up_bytes(solver.operands_bit_length, dd->get_alignment().boundary);
+        }
+        const auto o = op->access_data_def()->get_operand_value(solver);
+        const auto* dd_op = dynamic_cast<checking::data_definition_operand*>(o.get());
+
+
+        if (!dd_op->check<instr_type>(diagnostic_collector()))
+            return 0;
+
+        solver.operands_bit_length += dd_op->get_length();
+    }
+
+    // align to whole byte
+    round_up_bytes(solver.operands_bit_length, 1);
+
+    // returns the length in bytes
+    uint64_t len = solver.operands_bit_length / 8;
+
+    if (len > std::numeric_limits<int32_t>::max())
+        return 0;
+    else
+        return (int32_t)len;
+}
+
+template<checking::data_instr_type instr_type>
+context::symbol_value data_def_dependency<instr_type>::resolve(context::dependency_solver& solver) const
+{
+    return get_operands_length(m_begin, m_end, solver, &m_loctr);
+}
+
+template class data_def_postponed_statement<checking::data_instr_type::DC>;
+template class data_def_postponed_statement<checking::data_instr_type::DS>;
+template class data_def_dependency<checking::data_instr_type::DC>;
+template class data_def_dependency<checking::data_instr_type::DS>;
+
+const context::symbol* data_def_dependency_solver::get_symbol(context::id_index name) const
+{
+    return base.get_symbol(name);
+}
+
+std::optional<context::address> data_def_dependency_solver::get_loctr() const
+{
+    if (loctr)
+        return *loctr + (int)(operands_bit_length / 8);
+    if (auto l = base.get_loctr(); l.has_value())
+        return l.value() + (int)(operands_bit_length / 8);
+
+    return std::nullopt;
+}
+
+context::id_index data_def_dependency_solver::get_literal_id(const std::string& text,
+    const std::shared_ptr<const expressions::data_definition>& dd,
+    const range& r,
+    bool align_on_halfword)
+{
+    return base.get_literal_id(text, dd, r, align_on_halfword);
+}
+
+} // namespace hlasm_plugin::parser_library::processing