Rework OMF sections to include COMDATs; fix Borland/Watcom parsing

- Fix SEGDEF length field size to depend on record type, not the P (use32)
  bit, and honor the B (big) bit. This fixes parsing of Borland objects.
- Parse COMDAT records per the TIS spec (separate align byte, conditional
  public base fields), synthesize sections and symbols for them, and
  support continuation and iterated data.
- Allow FIXUPP records to follow COMDAT records.
- Support Borland virtual segments (COMDEF with a segment index data type,
  referenced via segment indices with bit 14 set).
- Read the FIXUP M bit from the correct byte (LOCAT instead of fix data).
- Fix iterated data (LIDATA) repeat/block counts to be plain integers
  rather than COMDEF-style encoded values, support multiple consecutive
  blocks, and propagate expansion errors.
- Fix relocation section targets to use 1-based section indices.
- Rework relocation kind mapping: self-relative fixups map to Relative
  with an end-of-location addend; segment-relative offsets map to Absolute
  (FLAT frame) or SectionOffset (target-section frame).
- Return empty imports/exports like other relocatable formats.

src/read/omf/comdat.rs

@@ -1,44 +1,41 @@
 use crate::read::{self, Error, Result};
-use crate::{omf, ComdatKind, ObjectComdat, ReadRef, SectionIndex, SymbolIndex};
+use crate::{ComdatKind, ObjectComdat, ReadRef, SectionIndex, SymbolIndex};
 
 use super::OmfFile;
 
-/// A COMDAT (communal data) section
+/// Internal representation of a COMDAT.
 #[derive(Debug, Clone)]
 pub(super) struct OmfComdatData<'data> {
     /// Symbol name
     pub(super) name: &'data [u8],
-    /// Segment index where this COMDAT belongs
-    pub(super) segment_index: u16,
-    /// Selection/allocation method
+    /// Index of the synthesized section (0-based index into `sections`)
+    pub(super) section: usize,
+    /// Index of the synthesized symbol
+    pub(super) symbol: SymbolIndex,
+    /// Selection criteria
     pub(super) selection: OmfComdatSelection,
-    /// Alignment
-    #[allow(unused)]
-    pub(super) alignment: omf::SegmentAlignment,
-    /// Data
-    #[allow(unused)]
-    pub(super) data: &'data [u8],
 }
 
-/// COMDAT selection methods
+/// COMDAT selection criteria
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub(super) enum OmfComdatSelection {
     /// Explicit: may not be combined, produce error if multiple definitions
-    Explicit = 0,
+    Explicit,
     /// Use any: pick any instance
-    UseAny = 1,
+    UseAny,
     /// Same size: all instances must be same size
-    SameSize = 2,
+    SameSize,
     /// Exact match: all instances must have identical content
-    ExactMatch = 3,
+    ExactMatch,
 }
 
-/// A COMDAT section in an OMF file.
+/// A COMDAT section group in an [`OmfFile`].
+///
+/// Most functionality is provided by the [`ObjectComdat`] trait implementation.
 #[derive(Debug)]
 pub struct OmfComdat<'data, 'file, R: ReadRef<'data>> {
     file: &'file OmfFile<'data, R>,
     index: usize,
-    _phantom: core::marker::PhantomData<&'data ()>,
 }
 
 impl<'data, 'file, R: ReadRef<'data>> read::private::Sealed for OmfComdat<'data, 'file, R> {}
@@ -47,8 +44,7 @@ impl<'data, 'file, R: ReadRef<'data>> ObjectComdat<'data> for OmfComdat<'data, '
     type SectionIterator = OmfComdatSectionIterator<'data, 'file, R>;
 
     fn kind(&self) -> ComdatKind {
-        let comdat = &self.file.comdats[self.index];
-        match comdat.selection {
+        match self.file.comdats[self.index].selection {
             OmfComdatSelection::Explicit => ComdatKind::NoDuplicates,
             OmfComdatSelection::UseAny => ComdatKind::Any,
             OmfComdatSelection::SameSize => ComdatKind::SameSize,
@@ -57,31 +53,27 @@ impl<'data, 'file, R: ReadRef<'data>> ObjectComdat<'data> for OmfComdat<'data, '
     }
 
     fn symbol(&self) -> SymbolIndex {
-        // COMDAT symbols don't have a direct symbol index in OMF
-        SymbolIndex(usize::MAX)
+        self.file.comdats[self.index].symbol
     }
 
     fn name_bytes(&self) -> Result<&'data [u8]> {
-        let comdat = &self.file.comdats[self.index];
-        Ok(comdat.name)
+        Ok(self.file.comdats[self.index].name)
     }
 
     fn name(&self) -> Result<&'data str> {
-        let comdat = &self.file.comdats[self.index];
-        core::str::from_utf8(comdat.name).map_err(|_| Error("Invalid UTF-8 in COMDAT name"))
+        core::str::from_utf8(self.file.comdats[self.index].name)
+            .map_err(|_| Error("Invalid UTF-8 in COMDAT name"))
     }
 
     fn sections(&self) -> Self::SectionIterator {
-        let comdat = &self.file.comdats[self.index];
         OmfComdatSectionIterator {
-            segment_index: (comdat.segment_index as usize).checked_sub(1),
-            returned: false,
+            section: Some(SectionIndex(self.file.comdats[self.index].section + 1)),
             _phantom: core::marker::PhantomData,
         }
     }
 }
 
-/// An iterator over COMDAT sections.
+/// An iterator for the COMDAT section groups in an [`OmfFile`].
 #[derive(Debug)]
 pub struct OmfComdatIterator<'data, 'file, R: ReadRef<'data>> {
     pub(super) file: &'file OmfFile<'data, R>,
@@ -96,7 +88,6 @@ impl<'data, 'file, R: ReadRef<'data>> Iterator for OmfComdatIterator<'data, 'fil
             let comdat = OmfComdat {
                 file: self.file,
                 index: self.index,
-                _phantom: core::marker::PhantomData,
             };
             self.index += 1;
             Some(comdat)
@@ -106,11 +97,10 @@ impl<'data, 'file, R: ReadRef<'data>> Iterator for OmfComdatIterator<'data, 'fil
     }
 }
 
-/// An iterator over sections in a COMDAT.
+/// An iterator for the sections in a [`OmfComdat`].
 #[derive(Debug)]
 pub struct OmfComdatSectionIterator<'data, 'file, R: ReadRef<'data>> {
-    segment_index: Option<usize>,
-    returned: bool,
+    section: Option<SectionIndex>,
     _phantom: core::marker::PhantomData<(&'data (), &'file (), R)>,
 }
 
@@ -118,11 +108,6 @@ impl<'data, 'file, R: ReadRef<'data>> Iterator for OmfComdatSectionIterator<'dat
     type Item = SectionIndex;
 
     fn next(&mut self) -> Option<Self::Item> {
-        if !self.returned {
-            self.returned = true;
-            self.segment_index.map(|idx| SectionIndex(idx + 1))
-        } else {
-            None
-        }
+        self.section.take()
     }
 }

src/read/omf/relocation.rs

@@ -1,7 +1,7 @@
 use crate::read::ReadRef;
 use crate::{
     omf, Relocation, RelocationEncoding, RelocationFlags, RelocationKind, RelocationTarget,
-    SectionIndex, SymbolIndex,
+    SectionIndex,
 };
 
 use super::{FrameMethod, OmfFile, TargetMethod};
@@ -9,7 +9,7 @@ use super::{FrameMethod, OmfFile, TargetMethod};
 /// An OMF fixup (relocation entry).
 #[derive(Debug, Clone)]
 pub(super) struct OmfFixup {
-    /// Offset in segment where fixup is applied
+    /// Offset in the section where the fixup is applied
     pub(super) offset: u32,
     /// Location type (what to patch)
     pub(super) location: omf::FixupLocation,
@@ -23,97 +23,122 @@ pub(super) struct OmfFixup {
     pub(super) target_index: u16,
     /// Target displacement
     pub(super) target_displacement: u32,
-    /// M-bit: true for segment-relative, false for PC-relative
+    /// M-bit: true for segment-relative, false for self-relative
     pub(super) is_segment_relative: bool,
 }
 
-/// An iterator over OMF relocations.
+impl omf::FixupLocation {
+    /// The size in bytes of the location being fixed up.
+    fn byte_size(self) -> u8 {
+        match self {
+            omf::FixupLocation::LowByte | omf::FixupLocation::HighByte => 1,
+            omf::FixupLocation::Offset
+            | omf::FixupLocation::LoaderOffset
+            | omf::FixupLocation::Base => 2,
+            omf::FixupLocation::Pointer
+            | omf::FixupLocation::Offset32
+            | omf::FixupLocation::LoaderOffset32 => 4,
+            omf::FixupLocation::Pointer48 => 6,
+        }
+    }
+}
+
+/// An iterator for the relocations in an [`OmfSection`](super::OmfSection).
 #[derive(Debug)]
 pub struct OmfRelocationIterator<'data, 'file, R: ReadRef<'data>> {
     pub(super) file: &'file OmfFile<'data, R>,
-    pub(super) segment_index: usize,
+    pub(super) section_index: usize,
     pub(super) index: usize,
 }
 
+impl<'data, 'file, R: ReadRef<'data>> OmfRelocationIterator<'data, 'file, R> {
+    /// Resolve a 1-based segment index to a section index.
+    fn target_section(&self, segment_index: u16) -> Option<SectionIndex> {
+        let section = self.file.segdef_section(segment_index).ok()?;
+        Some(SectionIndex(section + 1))
+    }
+
+    /// Return true if the frame of the fixup is the FLAT group, in which case
+    /// segment-relative fixups resolve to linear addresses.
+    fn frame_is_flat(&self, reloc: &OmfFixup) -> bool {
+        reloc.frame_method == FrameMethod::GroupIndex
+            && self.file.group_name(reloc.frame_index) == Some(b"FLAT")
+    }
+}
+
 impl<'data, 'file, R: ReadRef<'data>> Iterator for OmfRelocationIterator<'data, 'file, R> {
     type Item = (u64, Relocation);
 
     fn next(&mut self) -> Option<Self::Item> {
-        let relocations = &self.file.segments[self.segment_index].relocations;
+        let relocations = &self.file.sections[self.section_index].relocations;
         let reloc = relocations.get(self.index)?;
         self.index += 1;
 
-        let (mut kind, size, base_addend) = match reloc.location {
-            omf::FixupLocation::LowByte => (RelocationKind::Absolute, 8, 0),
-            omf::FixupLocation::HighByte => (RelocationKind::Absolute, 8, 0),
-            omf::FixupLocation::Offset | omf::FixupLocation::LoaderOffset => {
-                if reloc.is_segment_relative {
-                    (RelocationKind::SectionOffset, 16, 0)
-                } else {
-                    (RelocationKind::Relative, 16, -2)
-                }
-            }
-            omf::FixupLocation::Offset32 | omf::FixupLocation::LoaderOffset32 => {
-                if reloc.is_segment_relative {
-                    (RelocationKind::SectionOffset, 32, 0)
-                } else {
-                    (RelocationKind::Relative, 32, -4)
-                }
-            }
-            omf::FixupLocation::Base => {
-                if matches!(reloc.target_method, TargetMethod::SegmentIndex) {
-                    (RelocationKind::SectionIndex, 16, 0)
-                } else {
-                    (RelocationKind::Unknown, 16, 0)
-                }
-            }
-            omf::FixupLocation::Pointer => (RelocationKind::Absolute, 32, 0),
-            omf::FixupLocation::Pointer48 => (RelocationKind::Absolute, 48, 0),
-        };
-
-        if matches!(kind, RelocationKind::SectionOffset)
-            && !matches!(reloc.target_method, TargetMethod::SegmentIndex)
-        {
-            kind = RelocationKind::Unknown;
-        }
-
-        if matches!(
-            reloc.location,
-            omf::FixupLocation::LoaderOffset | omf::FixupLocation::LoaderOffset32
-        ) && matches!(reloc.frame_method, FrameMethod::ExternalIndex)
-        {
-            kind = RelocationKind::Unknown;
-        }
-
-        if matches!(reloc.target_method, TargetMethod::GroupIndex) {
-            kind = RelocationKind::Unknown;
-        }
+        let size = reloc.location.byte_size() * 8;
 
         let target = match reloc.target_method {
-            TargetMethod::SegmentIndex => {
-                if let Some(zero_based) = reloc.target_index.checked_sub(1) {
-                    let index = zero_based as usize;
-                    if index < self.file.segments.len() {
-                        RelocationTarget::Section(SectionIndex(index))
-                    } else {
-                        RelocationTarget::Absolute
-                    }
-                } else {
-                    RelocationTarget::Absolute
-                }
-            }
+            TargetMethod::SegmentIndex => match self.target_section(reloc.target_index) {
+                Some(section) => RelocationTarget::Section(section),
+                None => RelocationTarget::Absolute,
+            },
             TargetMethod::ExternalIndex => {
-                // External indices in OMF are 1-based indices into the external-name table
-                if let Some(symbol) = self.file.external_symbol(reloc.target_index) {
-                    RelocationTarget::Symbol(SymbolIndex(symbol.symbol_index))
-                } else {
-                    RelocationTarget::Absolute
+                // External indices are 1-based indices into the external-name table
+                match self.file.external_symbol(reloc.target_index) {
+                    Some(symbol) => RelocationTarget::Symbol(symbol.index),
+                    None => RelocationTarget::Absolute,
                 }
             }
             TargetMethod::GroupIndex | TargetMethod::FrameNumber => RelocationTarget::Absolute,
         };
 
-        let fixup_frame = match reloc.frame_method {
+        let mut addend = reloc.target_displacement as i64;
+        let kind = if !reloc.is_segment_relative {
+            // Self-relative fixups are relative to the end of the location.
+            addend -= reloc.location.byte_size() as i64;
+            RelocationKind::Relative
+        } else {
+            match reloc.location {
+                // The segment portion of the target's address.
+                omf::FixupLocation::Base => {
+                    if reloc.target_method == TargetMethod::SegmentIndex {
+                        RelocationKind::SectionIndex
+                    } else {
+                        RelocationKind::Unknown
+                    }
+                }
+                // Far pointers hold a complete segment:offset address.
+                omf::FixupLocation::Pointer | omf::FixupLocation::Pointer48 => {
+                    RelocationKind::Absolute
+                }
+                // The offset portion of the target's address, relative to the
+                // frame.
+                omf::FixupLocation::LowByte
+                | omf::FixupLocation::Offset
+                | omf::FixupLocation::LoaderOffset
+                | omf::FixupLocation::Offset32
+                | omf::FixupLocation::LoaderOffset32 => {
+                    if self.frame_is_flat(reloc) {
+                        // The FLAT group has a base address of 0, so the
+                        // offset is a linear address.
+                        RelocationKind::Absolute
+                    } else if reloc.frame_method == FrameMethod::Target
+                        || (reloc.frame_method == FrameMethod::SegmentIndex
+                            && reloc.target_method == TargetMethod::SegmentIndex
+                            && reloc.frame_index == reloc.target_index)
+                    {
+                        // The frame is the section containing the target, so
+                        // the offset is a section offset.
+                        RelocationKind::SectionOffset
+                    } else {
+                        RelocationKind::Unknown
+                    }
+                }
+                // The high byte of an offset can't be expressed generically.
+                omf::FixupLocation::HighByte => RelocationKind::Unknown,
+            }
+        };
+
+        let frame = match reloc.frame_method {
             FrameMethod::SegmentIndex => omf::FixupFrame::Segment(reloc.frame_index),
             FrameMethod::GroupIndex => omf::FixupFrame::Group(reloc.frame_index),
             FrameMethod::ExternalIndex => omf::FixupFrame::External(reloc.frame_index),
@@ -134,7 +159,7 @@ impl<'data, 'file, R: ReadRef<'data>> Iterator for OmfRelocationIterator<'data,
             encoding: RelocationEncoding::Generic,
             size,
             target,
-            addend: (reloc.target_displacement as i64) + base_addend,
+            addend,
             implicit_addend: false,
             subtractor: None,
             flags: RelocationFlags::Omf {
@@ -144,9 +169,8 @@ impl<'data, 'file, R: ReadRef<'data>> Iterator for OmfRelocationIterator<'data,
                 } else {
                     omf::FixupMode::SelfRelative
                 },
-                frame: fixup_frame,
+                frame,
                 target: fixup_target,
-                // target_displacement: reloc.target_displacement,
             },
         };
 

src/read/omf/section.rs

@@ -1,5 +1,5 @@
 use alloc::borrow::Cow;
-use alloc::{vec, vec::Vec};
+use alloc::vec::Vec;
 use core::str;
 
 use crate::read::{
@@ -7,9 +7,12 @@ use crate::read::{
     Result, SectionFlags, SectionIndex, SectionKind,
 };
 
-use super::{expand_lidata_block, OmfDataChunk, OmfFile, OmfRelocationIterator, OmfSegment};
+use super::{OmfDataChunk, OmfFile, OmfRelocationIterator, OmfSegment};
 
-/// A section in an OMF file.
+/// A section in an [`OmfFile`].
+///
+/// This is either a segment from a SEGDEF record, or a section synthesized
+/// from COMDAT records.
 #[derive(Debug)]
 pub struct OmfSection<'data, 'file, R: ReadRef<'data>> {
     pub(super) file: &'file OmfFile<'data, R>,
@@ -18,17 +21,17 @@ pub struct OmfSection<'data, 'file, R: ReadRef<'data>> {
 
 /// An OMF group definition
 #[derive(Debug, Clone)]
-#[allow(unused)]
 pub(super) struct OmfGroup {
     /// Group name index (into names table)
     pub(super) name_index: u16,
-    /// Segment indices in this group
+    /// Segment indices in this group (1-based SEGDEF indices)
+    #[allow(unused)]
     pub(super) segments: Vec<u16>,
 }
 
 impl<'data, 'file, R: ReadRef<'data>> OmfSection<'data, 'file, R> {
-    fn segment(&self) -> &OmfSegment<'data> {
-        &self.file.segments[self.index]
+    fn segment(&self) -> &'file OmfSegment<'data> {
+        &self.file.sections[self.index]
     }
 }
 
@@ -46,42 +49,20 @@ impl<'data, 'file, R: ReadRef<'data>> ObjectSection<'data> for OmfSection<'data,
     }
 
     fn size(&self) -> u64 {
-        self.segment().length as u64
+        self.segment().length
     }
 
     fn align(&self) -> u64 {
-        match self.segment().alignment {
-            crate::omf::SegmentAlignment::Byte => 1,
-            crate::omf::SegmentAlignment::Word => 2,
-            crate::omf::SegmentAlignment::Paragraph => 16,
-            crate::omf::SegmentAlignment::Page => 256,
-            crate::omf::SegmentAlignment::DWord => 4,
-            crate::omf::SegmentAlignment::Page4K => 4096,
-            _ => 1,
-        }
+        self.segment().align_bytes()
     }
 
     fn file_range(&self) -> Option<(u64, u64)> {
+        // OMF section data is not contiguous in the file.
         None
     }
 
     fn data(&self) -> Result<&'data [u8]> {
-        let segment = self.segment();
-
-        // Check if we have a single contiguous chunk that doesn't need expansion
-        if let Some(data) = segment.get_single_chunk() {
-            return Ok(data);
-        }
-
-        // If we have no chunks, return empty slice
-        if segment.data_chunks.is_empty() {
-            return Ok(&[]);
-        }
-
-        // For multiple chunks, LIDATA, or non-contiguous data, we can't return a reference
-        Err(Error(
-            "OMF segment data is not contiguous; use uncompressed_data() instead",
-        ))
+        self.segment().data()
     }
 
     fn data_range(&self, address: u64, size: u64) -> Result<Option<&'data [u8]>> {
@@ -91,31 +72,18 @@ impl<'data, 'file, R: ReadRef<'data>> ObjectSection<'data> for OmfSection<'data,
             .checked_add(size as usize)
             .ok_or(Error("Invalid data range"))?;
 
-        // Check if we have a single contiguous chunk that covers the range
-        if let Some(data) = segment.get_single_chunk() {
-            if offset > data.len() || end > data.len() {
-                return Ok(None);
-            }
-            return Ok(Some(&data[offset..end]));
-        }
-
-        // For multiple chunks, check if the requested range is within a single chunk
+        // Check if the requested range is within a single direct chunk.
         for (chunk_offset, chunk) in &segment.data_chunks {
             let chunk_start = *chunk_offset as usize;
-
-            // Only handle direct data chunks for now
             if let OmfDataChunk::Direct(chunk_data) = chunk {
                 let chunk_end = chunk_start + chunk_data.len();
-
                 if offset >= chunk_start && end <= chunk_end {
-                    let relative_offset = offset - chunk_start;
-                    let relative_end = end - chunk_start;
-                    return Ok(Some(&chunk_data[relative_offset..relative_end]));
+                    return Ok(Some(&chunk_data[offset - chunk_start..end - chunk_start]));
                 }
             }
         }
 
-        // Range spans multiple chunks, includes LIDATA, or is not available
+        // Range spans multiple chunks, includes iterated data, or is not available.
         Ok(None)
     }
 
@@ -129,59 +97,22 @@ impl<'data, 'file, R: ReadRef<'data>> ObjectSection<'data> for OmfSection<'data,
 
     fn uncompressed_data(&self) -> Result<Cow<'data, [u8]>> {
         let segment = self.segment();
-
-        // Check if we have a single contiguous chunk that doesn't need expansion
-        if let Some(data) = segment.get_single_chunk() {
-            return Ok(Cow::Borrowed(data));
-        }
-
-        // If we have no chunks, return empty
         if segment.data_chunks.is_empty() {
             return Ok(Cow::Borrowed(&[]));
         }
-
-        // We need to construct the full segment data
-        let mut result = vec![0u8; segment.length as usize];
-
-        for (offset, chunk) in &segment.data_chunks {
-            let start = *offset as usize;
-
-            match chunk {
-                OmfDataChunk::Direct(data) => {
-                    // Direct data
-                    let end = start + data.len();
-                    if end <= result.len() {
-                        result[start..end].copy_from_slice(data);
-                    } else {
-                        return Err(Error("OMF segment data chunk exceeds segment length"));
-                    }
-                }
-                OmfDataChunk::Iterated(lidata) => {
-                    // LIDATA needs expansion
-                    if let Ok(expanded) = expand_lidata_block(lidata) {
-                        let end = start + expanded.len();
-                        if end <= result.len() {
-                            result[start..end].copy_from_slice(&expanded);
-                        } else {
-                            return Err(Error("OMF LIDATA expansion exceeds segment length"));
-                        }
-                    }
-                }
-            }
+        if let Some(data) = segment.single_chunk() {
+            return Ok(Cow::Borrowed(data));
         }
-
-        Ok(Cow::Owned(result))
+        // The data is non-contiguous or iterated, so it must be copied.
+        segment.build_data().map(Cow::Owned)
     }
 
     fn name_bytes(&self) -> Result<&'data [u8]> {
-        let segment = self.segment();
-        self.file
-            .get_name(segment.name_index)
-            .ok_or(Error("Invalid segment name index"))
+        Ok(self.segment().name)
     }
 
     fn name(&self) -> Result<&'data str> {
-        str::from_utf8(self.name_bytes()?).map_err(|_| Error("Invalid UTF-8 in segment name"))
+        str::from_utf8(self.name_bytes()?).map_err(|_| Error("Invalid UTF-8 in OMF section name"))
     }
 
     fn segment_name_bytes(&self) -> Result<Option<&'data [u8]>> {
@@ -193,13 +124,13 @@ impl<'data, 'file, R: ReadRef<'data>> ObjectSection<'data> for OmfSection<'data,
     }
 
     fn kind(&self) -> SectionKind {
-        self.file.segment_section_kind(self.index)
+        self.file.section_kind(self.index)
     }
 
     fn relocations(&self) -> Self::RelocationIterator {
         OmfRelocationIterator {
             file: self.file,
-            segment_index: self.index,
+            section_index: self.index,
             index: 0,
         }
     }
@@ -209,25 +140,11 @@ impl<'data, 'file, R: ReadRef<'data>> ObjectSection<'data> for OmfSection<'data,
     }
 
     fn flags(&self) -> SectionFlags {
-        let segment = self.segment();
-        let flags = SectionFlags::None;
-
-        // Set flags based on segment properties
-        match segment.combination {
-            crate::omf::SegmentCombination::Public => {
-                // Public segments are like COMDAT sections
-            }
-            crate::omf::SegmentCombination::Stack => {
-                // Stack segments
-            }
-            _ => {}
-        }
-
-        flags
+        SectionFlags::None
     }
 }
 
-/// An iterator over OMF sections.
+/// An iterator for the sections in an [`OmfFile`].
 #[derive(Debug)]
 pub struct OmfSectionIterator<'data, 'file, R: ReadRef<'data>> {
     pub(super) file: &'file OmfFile<'data, R>,
@@ -238,7 +155,7 @@ impl<'data, 'file, R: ReadRef<'data>> Iterator for OmfSectionIterator<'data, 'fi
     type Item = OmfSection<'data, 'file, R>;
 
     fn next(&mut self) -> Option<Self::Item> {
-        if self.index < self.file.segments.len() {
+        if self.index < self.file.sections.len() {
             let section = OmfSection {
                 file: self.file,
                 index: self.index,

src/read/omf/segment.rs

@@ -1,75 +1,207 @@
 use alloc::vec::Vec;
 
-use crate::read::{self, ObjectSegment, ReadRef, Result, SectionKind};
+use crate::read::{self, Error, ObjectSegment, ReadRef, Result, SectionKind};
 use crate::{omf, Permissions, SegmentFlags};
 
-use super::{OmfFile, OmfFixup};
+use super::{expand_iterated_data, iterated_data_expanded_len, OmfFile, OmfFixup};
 
-/// An OMF segment definition
+/// A section in an OMF file.
+///
+/// This is either a segment from a SEGDEF record, or a section synthesized
+/// from COMDAT records.
 #[derive(Debug, Clone)]
 pub struct OmfSegment<'data> {
-    /// Segment name index (into names table)
+    /// Segment name.
+    ///
+    /// For sections synthesized from COMDAT or Borland COMDEF records, this
+    /// is the symbol name.
+    pub(super) name: &'data [u8],
+    /// Class name (resolved from the names table)
+    pub(super) class: &'data [u8],
+    /// Segment name index (into names table), or 0 if the name doesn't come
+    /// from the names table.
     pub(super) name_index: u16,
     /// Class name index (into names table)
     pub(super) class_index: u16,
     /// Overlay name index (into names table)
-    #[allow(unused)] // TODO
     pub(super) overlay_index: u16,
     /// Segment alignment
     pub(super) alignment: omf::SegmentAlignment,
     /// Segment combination
     pub(super) combination: omf::SegmentCombination,
     /// Whether this is a 32-bit segment
-    #[allow(unused)] // TODO
     pub(super) use32: bool,
-    /// Segment length
-    pub(super) length: u32,
+    /// Segment length in bytes
+    pub(super) length: u64,
+    /// Section kind for COMDAT sections with non-explicit allocation
+    pub(super) kind: Option<SectionKind>,
+    /// Whether this section was synthesized from COMDAT records
+    pub(super) comdat: bool,
     /// Segment data chunks (offset, data)
-    /// Multiple LEDATA/LIDATA records can contribute to a single segment
+    ///
+    /// Multiple LEDATA/LIDATA/COMDAT records can contribute to a single section.
     pub(super) data_chunks: Vec<(u32, OmfDataChunk<'data>)>,
-    /// Relocations for this segment
+    /// Relocations for this section
     pub(super) relocations: Vec<OmfFixup>,
 }
 
-/// Data chunk for a segment
+/// Data chunk for a section
 #[derive(Debug, Clone)]
 pub(super) enum OmfDataChunk<'data> {
-    /// Direct data from LEDATA record
+    /// Direct data from a LEDATA or COMDAT record
     Direct(&'data [u8]),
-    /// Compressed/iterated data from LIDATA record (needs expansion)
-    Iterated(&'data [u8]),
+    /// Iterated data from a LIDATA or COMDAT record (needs expansion)
+    Iterated {
+        data: &'data [u8],
+        /// Whether repeat counts are 32-bit (from a 32-bit record type)
+        is_32bit: bool,
+    },
+}
+
+impl<'data> OmfDataChunk<'data> {
+    /// Get the size of the chunk after expansion
+    pub(super) fn expanded_len(&self) -> Result<u64> {
+        match *self {
+            OmfDataChunk::Direct(data) => Ok(data.len() as u64),
+            OmfDataChunk::Iterated { data, is_32bit } => iterated_data_expanded_len(data, is_32bit),
+        }
+    }
 }
 
 impl<'data> OmfSegment<'data> {
-    /// Get the raw data of the segment if it's a single contiguous chunk
-    pub fn get_single_chunk(&self) -> Option<&'data [u8]> {
-        if self.data_chunks.len() == 1 {
-            let (offset, chunk) = &self.data_chunks[0];
-            if *offset == 0 {
-                match chunk {
-                    OmfDataChunk::Direct(data) if data.len() == self.length as usize => {
-                        return Some(data);
-                    }
-                    _ => {}
-                }
+    /// Get the segment name.
+    pub fn name(&self) -> &'data [u8] {
+        self.name
+    }
+
+    /// Get the class name.
+    pub fn class(&self) -> &'data [u8] {
+        self.class
+    }
+
+    /// Get the segment name index (into the names table).
+    ///
+    /// For COMDAT sections, this is the public name index. This is 0 for
+    /// sections whose name doesn't come from the names table.
+    pub fn name_index(&self) -> u16 {
+        self.name_index
+    }
+
+    /// Get the class name index (into the names table).
+    ///
+    /// This is 0 for COMDAT sections without a base segment.
+    pub fn class_index(&self) -> u16 {
+        self.class_index
+    }
+
+    /// Get the overlay name index (into the names table).
+    pub fn overlay_index(&self) -> u16 {
+        self.overlay_index
+    }
+
+    /// Get the segment alignment.
+    pub fn alignment(&self) -> omf::SegmentAlignment {
+        self.alignment
+    }
+
+    /// Get the segment combination.
+    pub fn combination(&self) -> omf::SegmentCombination {
+        self.combination
+    }
+
+    /// Return true if this is a 32-bit segment.
+    pub fn use32(&self) -> bool {
+        self.use32
+    }
+
+    /// Get the segment length in bytes.
+    pub fn length(&self) -> u64 {
+        self.length
+    }
+
+    /// Return true if this section was synthesized from COMDAT records.
+    pub fn is_comdat(&self) -> bool {
+        self.comdat
+    }
+
+    /// Get the alignment in bytes.
+    pub(super) fn align_bytes(&self) -> u64 {
+        match self.alignment {
+            omf::SegmentAlignment::Absolute => 1,
+            omf::SegmentAlignment::Byte => 1,
+            omf::SegmentAlignment::Word => 2,
+            omf::SegmentAlignment::Paragraph => 16,
+            omf::SegmentAlignment::Page => 256,
+            omf::SegmentAlignment::DWord => 4,
+            omf::SegmentAlignment::Page4K => 4096,
+        }
+    }
+
+    /// Get the raw data of the section if it's a single contiguous chunk
+    pub(super) fn single_chunk(&self) -> Option<&'data [u8]> {
+        if let [(0, OmfDataChunk::Direct(data))] = self.data_chunks[..] {
+            if data.len() as u64 == self.length {
+                return Some(data);
             }
         }
         None
     }
 
-    /// Check if any data chunk needs expansion (LIDATA)
-    pub fn has_iterated_data(&self) -> bool {
-        self.data_chunks
-            .iter()
-            .any(|(_, chunk)| matches!(chunk, OmfDataChunk::Iterated(_)))
+    /// Get the data of the section if it can be returned without copying.
+    ///
+    /// Returns an error if the data is non-contiguous or requires expansion.
+    pub(super) fn data(&self) -> Result<&'data [u8]> {
+        if self.data_chunks.is_empty() {
+            return Ok(&[]);
+        }
+        self.single_chunk().ok_or(Error(
+            "OMF section data is not contiguous; use uncompressed_data() instead",
+        ))
+    }
+
+    /// Build the complete section data, expanding iterated data if needed.
+    pub(super) fn build_data(&self) -> Result<Vec<u8>> {
+        let length = usize::try_from(self.length).map_err(|_| Error("OMF section too large"))?;
+        let mut result = alloc::vec![0u8; length];
+
+        for (offset, chunk) in &self.data_chunks {
+            let start = *offset as usize;
+            match chunk {
+                OmfDataChunk::Direct(data) => {
+                    let end = start + data.len();
+                    if end > result.len() {
+                        return Err(Error("OMF section data chunk exceeds section length"));
+                    }
+                    result[start..end].copy_from_slice(data);
+                }
+                OmfDataChunk::Iterated { data, is_32bit } => {
+                    let expanded = expand_iterated_data(data, *is_32bit)?;
+                    let end = start + expanded.len();
+                    if end > result.len() {
+                        return Err(Error("OMF section data chunk exceeds section length"));
+                    }
+                    result[start..end].copy_from_slice(&expanded);
+                }
+            }
+        }
+
+        Ok(result)
     }
 }
 
-/// An OMF segment reference.
+/// A loadable section in an [`OmfFile`].
+///
+/// Most functionality is provided by the [`ObjectSegment`] trait implementation.
 #[derive(Debug)]
 pub struct OmfSegmentRef<'data, 'file, R: ReadRef<'data>> {
-    file: &'file OmfFile<'data, R>,
-    index: usize,
+    pub(super) file: &'file OmfFile<'data, R>,
+    pub(super) index: usize,
+}
+
+impl<'data, 'file, R: ReadRef<'data>> OmfSegmentRef<'data, 'file, R> {
+    fn segment(&self) -> &'file OmfSegment<'data> {
+        &self.file.sections[self.index]
+    }
 }
 
 impl<'data, 'file, R: ReadRef<'data>> read::private::Sealed for OmfSegmentRef<'data, 'file, R> {}
@@ -80,47 +212,33 @@ impl<'data, 'file, R: ReadRef<'data>> ObjectSegment<'data> for OmfSegmentRef<'da
     }
 
     fn size(&self) -> u64 {
-        self.file.segments[self.index].length as u64
+        self.segment().length
     }
 
     fn align(&self) -> u64 {
-        match self.file.segments[self.index].alignment {
-            crate::omf::SegmentAlignment::Byte => 1,
-            crate::omf::SegmentAlignment::Word => 2,
-            crate::omf::SegmentAlignment::Paragraph => 16,
-            crate::omf::SegmentAlignment::Page => 256,
-            crate::omf::SegmentAlignment::DWord => 4,
-            crate::omf::SegmentAlignment::Page4K => 4096,
-            _ => 1,
-        }
+        self.segment().align_bytes()
     }
 
     fn file_range(&self) -> (u64, u64) {
+        // OMF section data is not contiguous in the file.
         (0, 0)
     }
 
     fn data(&self) -> Result<&'data [u8]> {
-        // OMF segments don't have direct file mapping
-        Ok(&[])
+        self.segment().data()
     }
 
     fn data_range(&self, _address: u64, _size: u64) -> Result<Option<&'data [u8]>> {
+        // OMF sections have no address.
         Ok(None)
     }
 
     fn name_bytes(&self) -> Result<Option<&'data [u8]>> {
-        Ok(self
-            .file
-            .get_name(self.file.segments[self.index].name_index))
+        Ok(Some(self.segment().name))
     }
 
     fn name(&self) -> Result<Option<&'data str>> {
-        let index = self.file.segments[self.index].name_index;
-        let name_opt = self.file.get_name(index);
-        match name_opt {
-            Some(bytes) => Ok(core::str::from_utf8(bytes).ok()),
-            None => Ok(None),
-        }
+        Ok(core::str::from_utf8(self.segment().name).ok())
     }
 
     fn flags(&self) -> SegmentFlags {
@@ -129,8 +247,8 @@ impl<'data, 'file, R: ReadRef<'data>> ObjectSegment<'data> for OmfSegmentRef<'da
 
     fn permissions(&self) -> Permissions {
         // OMF segment definitions don't carry permission flags, so derive them
-        // from the segment's class name.
-        match self.file.segment_section_kind(self.index) {
+        // from the section kind.
+        match self.file.section_kind(self.index) {
             SectionKind::Text => Permissions::new(true, false, true),
             SectionKind::ReadOnlyData | SectionKind::ReadOnlyString | SectionKind::Debug => {
                 Permissions::new(true, false, false)
@@ -140,7 +258,7 @@ impl<'data, 'file, R: ReadRef<'data>> ObjectSegment<'data> for OmfSegmentRef<'da
     }
 }
 
-/// An iterator over OMF segments.
+/// An iterator for the loadable sections in an [`OmfFile`].
 #[derive(Debug)]
 pub struct OmfSegmentIterator<'data, 'file, R: ReadRef<'data>> {
     pub(super) file: &'file OmfFile<'data, R>,
@@ -151,7 +269,7 @@ impl<'data, 'file, R: ReadRef<'data>> Iterator for OmfSegmentIterator<'data, 'fi
     type Item = OmfSegmentRef<'data, 'file, R>;
 
     fn next(&mut self) -> Option<Self::Item> {
-        if self.index < self.file.segments.len() {
+        if self.index < self.file.sections.len() {
             let segment = OmfSegmentRef {
                 file: self.file,
                 index: self.index,

src/read/omf/symbol.rs

@@ -7,30 +7,34 @@ use crate::read::{
 
 use super::OmfFile;
 
-/// An OMF symbol
+/// A symbol in an [`OmfFile`].
+///
+/// Most functionality is provided by the [`ObjectSymbol`] trait implementation.
 #[derive(Debug, Clone)]
 pub struct OmfSymbol<'data> {
     /// Symbol table index
-    pub symbol_index: usize,
+    pub(super) index: SymbolIndex,
     /// Symbol name
-    pub name: &'data [u8],
+    pub(super) name: &'data [u8],
     /// Symbol class (Public, External, etc.)
-    pub class: OmfSymbolClass,
-    /// Group index (0 if none)
-    pub group_index: u16,
-    /// Segment index (0 if external)
-    pub segment_index: u16,
-    /// Frame number (for absolute symbols when segment_index == 0)
-    pub frame_number: u16,
-    /// Offset within segment
-    pub offset: u32,
+    pub(super) class: OmfSymbolClass,
+    /// The section that defines this symbol, if any
+    pub(super) section: Option<SectionIndex>,
+    /// Whether this is an absolute symbol (PUBDEF with segment index 0)
+    pub(super) absolute: bool,
+    /// Frame number (for absolute symbols)
+    pub(super) frame_number: u16,
+    /// Offset within the section
+    pub(super) offset: u64,
+    /// Symbol size (communal length for COMDEF, section size for COMDAT)
+    pub(super) size: u64,
     /// Type index (usually 0)
-    pub type_index: u16,
+    pub(super) type_index: u16,
     /// Pre-computed symbol kind
-    pub kind: SymbolKind,
+    pub(super) kind: SymbolKind,
 }
 
-/// Symbol class for OMF symbols
+/// The kind of OMF record that defined a symbol.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum OmfSymbolClass {
     /// Public symbol (PUBDEF)
@@ -47,13 +51,35 @@ pub enum OmfSymbolClass {
     LocalCommunal,
     /// COMDAT external symbol (CEXTDEF)
     ComdatExternal,
+    /// COMDAT symbol (COMDAT)
+    Comdat,
+    /// Local COMDAT symbol (COMDAT with the local flag)
+    LocalComdat,
+}
+
+impl<'data> OmfSymbol<'data> {
+    /// Get the symbol class, which corresponds to the kind of OMF record
+    /// that defined the symbol.
+    pub fn class(&self) -> OmfSymbolClass {
+        self.class
+    }
+
+    /// Get the type index.
+    pub fn type_index(&self) -> u16 {
+        self.type_index
+    }
+
+    /// Get the frame number for absolute symbols.
+    pub fn frame_number(&self) -> u16 {
+        self.frame_number
+    }
 }
 
 impl<'data> read::private::Sealed for OmfSymbol<'data> {}
 
 impl<'data> ObjectSymbol<'data> for OmfSymbol<'data> {
     fn index(&self) -> SymbolIndex {
-        SymbolIndex(self.symbol_index)
+        self.index
     }
 
     fn name_bytes(&self) -> Result<&'data [u8]> {
@@ -61,21 +87,21 @@ impl<'data> ObjectSymbol<'data> for OmfSymbol<'data> {
     }
 
     fn name(&self) -> Result<&'data str> {
-        core::str::from_utf8(self.name).map_err(|_| Error("Invalid UTF-8 in OMF symbol name"))
+        str::from_utf8(self.name).map_err(|_| Error("Invalid UTF-8 in OMF symbol name"))
     }
 
     fn address(&self) -> u64 {
-        if self.segment_index == 0 && self.frame_number != 0 {
-            // For absolute symbols, compute the linear address from frame:offset
-            // Frame number is in paragraphs (16-byte units)
-            ((self.frame_number as u64) << 4) + (self.offset as u64)
+        if self.absolute {
+            // For absolute symbols, compute the linear address from frame:offset.
+            // Frame number is in paragraphs (16-byte units).
+            ((self.frame_number as u64) << 4) + self.offset
         } else {
-            self.offset as u64
+            self.offset
         }
     }
 
     fn size(&self) -> u64 {
-        0 // OMF doesn't store symbol sizes
+        self.size
     }
 
     fn kind(&self) -> SymbolKind {
@@ -83,30 +109,37 @@ impl<'data> ObjectSymbol<'data> for OmfSymbol<'data> {
     }
 
     fn section(&self) -> SymbolSection {
-        if self.segment_index == 0 {
-            if self.frame_number != 0 {
-                SymbolSection::Absolute
-            } else {
-                SymbolSection::Undefined
-            }
+        if let Some(section) = self.section {
+            SymbolSection::Section(section)
+        } else if self.absolute {
+            SymbolSection::Absolute
+        } else if self.is_common() {
+            SymbolSection::Common
         } else {
-            SymbolSection::Section(SectionIndex(self.segment_index as usize))
+            SymbolSection::Undefined
         }
     }
 
     fn is_undefined(&self) -> bool {
-        self.segment_index == 0 && self.frame_number == 0
+        matches!(
+            self.class,
+            OmfSymbolClass::External
+                | OmfSymbolClass::LocalExternal
+                | OmfSymbolClass::ComdatExternal
+        )
     }
 
     fn is_definition(&self) -> bool {
-        self.segment_index != 0 || self.frame_number != 0
+        self.section.is_some() || self.absolute
     }
 
     fn is_common(&self) -> bool {
+        // Borland communal symbols with a virtual segment are defined in a
+        // section, so they are not common.
         matches!(
             self.class,
-            super::OmfSymbolClass::Communal | super::OmfSymbolClass::LocalCommunal
-        )
+            OmfSymbolClass::Communal | OmfSymbolClass::LocalCommunal
+        ) && self.section.is_none()
     }
 
     fn is_weak(&self) -> bool {
@@ -115,19 +148,14 @@ impl<'data> ObjectSymbol<'data> for OmfSymbol<'data> {
 
     fn scope(&self) -> SymbolScope {
         match self.class {
-            super::OmfSymbolClass::LocalPublic
-            | super::OmfSymbolClass::LocalExternal
-            | super::OmfSymbolClass::LocalCommunal => SymbolScope::Compilation,
-            super::OmfSymbolClass::Public
-            | super::OmfSymbolClass::External
-            | super::OmfSymbolClass::Communal
-            | super::OmfSymbolClass::ComdatExternal => {
-                if self.segment_index == 0 {
-                    SymbolScope::Unknown
-                } else {
-                    SymbolScope::Linkage
-                }
+            OmfSymbolClass::LocalPublic
+            | OmfSymbolClass::LocalExternal
+            | OmfSymbolClass::LocalCommunal
+            | OmfSymbolClass::LocalComdat => SymbolScope::Compilation,
+            OmfSymbolClass::Public | OmfSymbolClass::Communal | OmfSymbolClass::Comdat => {
+                SymbolScope::Linkage
             }
+            OmfSymbolClass::External | OmfSymbolClass::ComdatExternal => SymbolScope::Unknown,
         }
     }
 
@@ -138,9 +166,10 @@ impl<'data> ObjectSymbol<'data> for OmfSymbol<'data> {
     fn is_local(&self) -> bool {
         matches!(
             self.class,
-            super::OmfSymbolClass::LocalPublic
-                | super::OmfSymbolClass::LocalExternal
-                | super::OmfSymbolClass::LocalCommunal
+            OmfSymbolClass::LocalPublic
+                | OmfSymbolClass::LocalExternal
+                | OmfSymbolClass::LocalCommunal
+                | OmfSymbolClass::LocalComdat
         )
     }
 
@@ -149,7 +178,7 @@ impl<'data> ObjectSymbol<'data> for OmfSymbol<'data> {
     }
 }
 
-/// An iterator over OMF symbols.
+/// An iterator for the symbols in an [`OmfFile`].
 #[derive(Debug)]
 pub struct OmfSymbolIterator<'data, 'file, R: ReadRef<'data> = &'data [u8]> {
     pub(super) file: &'file OmfFile<'data, R>,
@@ -166,7 +195,7 @@ impl<'data, 'file, R: ReadRef<'data>> Iterator for OmfSymbolIterator<'data, 'fil
     }
 }
 
-/// An OMF symbol table.
+/// A symbol table in an [`OmfFile`].
 #[derive(Debug)]
 pub struct OmfSymbolTable<'data, 'file, R: ReadRef<'data>> {
     pub(super) file: &'file OmfFile<'data, R>,