PR feedback

Signed-off-by: Alan Jowett <[email protected]>
vbpf · Jan 23, 2025 · 320dee7 · 320dee7
1 parent e3dd273
commit 320dee7
Show file tree

Hide file tree

Showing 6 changed files with 28 additions and 46 deletions.
diff --git a/src/asm_files.cpp b/src/asm_files.cpp
@@ -372,14 +372,8 @@ vector<raw_program> read_elf(std::istream& input_stream, const std::string& path
 
     std::variant<size_t, std::map<std::string, size_t>> map_record_size_or_map_offsets = size_t{0};
     ELFIO::const_symbol_section_accessor symbols{reader, symbol_section};
-    bool contains_old_style_map_sections = false;
-    for (const auto& section : reader.sections) {
-        if (is_map_section(section->get_name())) {
-            contains_old_style_map_sections = true;
-            break;
-        }
-    }
-    if (contains_old_style_map_sections) {
+
+    if (std::ranges::any_of(reader.sections, [](const auto& section) { return is_map_section(section->get_name()); })) {
         map_record_size_or_map_offsets =
             parse_map_sections(options, platform, reader, info.map_descriptors, map_section_indices, symbols);
     } else if (btf_data.has_value()) {
@@ -408,16 +402,12 @@ vector<raw_program> read_elf(std::istream& input_stream, const std::string& path
             map_section_indices.insert(reader.sections[".maps"]->get_index());
         }
 
-        if (reader.sections[".data"]) {
-            global_variable_section_indices.insert(reader.sections[".data"]->get_index());
-        }
-
-        if (reader.sections[".bss"]) {
-            global_variable_section_indices.insert(reader.sections[".bss"]->get_index());
-        }
-
-        if (reader.sections[".rodata"]) {
-            global_variable_section_indices.insert(reader.sections[".rodata"]->get_index());
+        for (auto section_name : {".rodata", ".data", ".bss"}) {
+            if (const auto section = reader.sections[section_name]) {
+                if (section->get_size() != 0) {
+                    global_variable_section_indices.insert(section->get_index());
+                }
+            }
         }
     }
 

diff --git a/src/asm_marshal.cpp b/src/asm_marshal.cpp
@@ -299,7 +299,7 @@ vector<ebpf_inst> marshal(const Instruction& ins, const pc_t pc) {
     return std::visit(MarshalVisitor{crab::label_to_offset16(pc), crab::label_to_offset32(pc)}, ins);
 }
 
-static int size(const Instruction& inst) {
+int asm_syntax::size(const Instruction& inst) {
     if (const auto pins = std::get_if<Bin>(&inst)) {
         if (pins->lddw) {
             return 2;

diff --git a/src/asm_ostream.cpp b/src/asm_ostream.cpp
@@ -544,21 +544,6 @@ string to_string(Assertion const& constraint) {
     return str.str();
 }
 
-int size(const Instruction& inst) {
-    if (const auto bin = std::get_if<Bin>(&inst)) {
-        if (bin->lddw) {
-            return 2;
-        }
-    }
-    if (std::holds_alternative<LoadMapFd>(inst)) {
-        return 2;
-    }
-    if (std::holds_alternative<LoadMapAddress>(inst)) {
-        return 2;
-    }
-    return 1;
-}
-
 auto get_labels(const InstructionSeq& insts) {
     pc_t pc = 0;
     std::map<label_t, pc_t> pc_of_label;

diff --git a/src/asm_syntax.hpp b/src/asm_syntax.hpp
@@ -383,6 +383,8 @@ std::string to_string(const Assertion& constraint);
 void print(const InstructionSeq& insts, std::ostream& out, const std::optional<const label_t>& label_to_print,
            bool print_line_info = false);
 
+int size(const Instruction& inst);
+
 } // namespace asm_syntax
 
 using namespace asm_syntax;

diff --git a/src/asm_unmarshal.cpp b/src/asm_unmarshal.cpp
@@ -439,25 +439,26 @@ struct Unmarshaller {
             throw InvalidInstruction(pc, "bad register");
         }
 
-        if (inst.src == INST_LD_MODE_MAP_FD) {
+        switch (inst.src) {
+        case INST_LD_MODE_IMM:
+            return Bin{
+                .op = Bin::Op::MOV,
+                .dst = Reg{inst.dst},
+                .v = Imm{merge(inst.imm, next_imm)},
+                .is64 = true,
+                .lddw = true,
+            };
+        case INST_LD_MODE_MAP_FD: {
             // magic number, meaning we're a per-process file descriptor defining the map.
             // (for details, look for BPF_PSEUDO_MAP_FD in the kernel)
             if (next.imm != 0) {
                 throw InvalidInstruction(pc, "lddw uses reserved fields");
             }
             return LoadMapFd{.dst = Reg{inst.dst}, .mapfd = inst.imm};
         }
-        if (inst.src == INST_LD_MODE_MAP_VALUE) {
-            return LoadMapAddress{.dst = Reg{inst.dst}, .mapfd = inst.imm, .offset = next_imm};
+        case INST_LD_MODE_MAP_VALUE: return LoadMapAddress{.dst = Reg{inst.dst}, .mapfd = inst.imm, .offset = next_imm};
+        default: throw InvalidInstruction(pc, make_opcode_message("bad instruction", inst.opcode));
         }
-
-        return Bin{
-            .op = Bin::Op::MOV,
-            .dst = Reg{inst.dst},
-            .v = Imm{merge(inst.imm, next_imm)},
-            .is64 = true,
-            .lddw = true,
-        };
     }
 
     static ArgSingle::Kind toArgSingleKind(const ebpf_argument_type_t t) {

diff --git a/src/crab/ebpf_transformer.cpp b/src/crab/ebpf_transformer.cpp
@@ -1841,10 +1841,14 @@ void ebpf_transformer::do_load_mapfd(const Reg& dst_reg, const int mapfd, const
 
 void ebpf_transformer::operator()(const LoadMapFd& ins) { do_load_mapfd(ins.dst, ins.mapfd, false); }
 
-void ebpf_transformer::do_load_map_address(const Reg& dst_reg, const int mapfd, int32_t offset) {
+void ebpf_transformer::do_load_map_address(const Reg& dst_reg, const int mapfd, const int32_t offset) {
     const EbpfMapDescriptor& desc = thread_local_program_info->platform->get_map_descriptor(mapfd);
     const EbpfMapType& type = thread_local_program_info->platform->get_map_type(desc.type);
 
+    if (type.value_type == EbpfMapValueType::PROGRAM) {
+        throw std::runtime_error("Program map types are not supported");
+    }
+
     // Set the shared region size and offset for the map.
     type_inv.assign_type(m_inv, dst_reg, T_SHARED);
     const reg_pack_t& dst = reg_pack(dst_reg);