Skip to content

Commit d7b445a

Browse files
authored
fix: debug info alignment (#1468)
This PR reintroduces explicit alignments in our debug-info. Additionally, for struct-types, the offsets are now manually calculated and then compared with the LLVM-offset, choosing the bigger value. This prevents overlap when reading values from memory during debugging, which would lead to corrupted values. In particular, this issue arose for variables placed after 64-bit types. In the example below, the running offset didn't match the actual offset in memory. 304 + 8 != 308. `(gdb) ptype /o <struct>`: ``` /* 304 | 8 */ LINT var_LINT; /* 308 | 26 */ BYTE check_after_LINT[26]; ``` After reading a bit further into the LLVM documentation/performance guide for frontend authors, I have decided to enable alignment for each variable-type except globals. While alignment can safely be omitted for x86 architectures with little-to-no performance drawbacks, it is still recommended for MIPS and ARM ISAs. [When to specify alignment](https://llvm.org/docs/Frontend/PerformanceTips.html#when-to-specify-alignment) For global variables, however, it is recommended to let the target choose how it wants to align variables in memory. Specifying an alignment here would force the target to use the specified alignment and prevent targets and optimizers from over-aligning. [Global vars](https://llvm.org/docs/LangRef.html#globalvars) While these points are generally not written in regards to debug-information, I think i doesn't hurt to match these specifications when generating DI aswell.
1 parent f1fe5c0 commit d7b445a

File tree

39 files changed

+328
-237
lines changed

39 files changed

+328
-237
lines changed

compiler/plc_driver/src/tests/snapshots/plc_driver__tests__multi_files__multiple_files_in_different_locations_with_debug_info.snap

+4-3
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
---
22
source: compiler/plc_driver/./src/tests/multi_files.rs
33
expression: "results.join(\"\\n\")"
4+
snapshot_kind: text
45
---
56
; ModuleID = 'app/file1.st'
67
source_filename = "app/file1.st"
@@ -32,7 +33,7 @@ attributes #0 = { nofree nosync nounwind readnone speculatable willreturn }
3233
!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
3334
!1 = distinct !DIGlobalVariable(name: "mainProg", scope: !2, file: !2, line: 2, type: !3, isLocal: false, isDefinition: true)
3435
!2 = !DIFile(filename: "file2.st", directory: "lib")
35-
!3 = !DICompositeType(tag: DW_TAG_structure_type, name: "mainProg", scope: !2, file: !2, line: 2, flags: DIFlagPublic, elements: !4, identifier: "mainProg")
36+
!3 = !DICompositeType(tag: DW_TAG_structure_type, name: "mainProg", scope: !2, file: !2, line: 2, align: 64, flags: DIFlagPublic, elements: !4, identifier: "mainProg")
3637
!4 = !{}
3738
!5 = !{i32 2, !"Dwarf Version", i32 5}
3839
!6 = !{i32 2, !"Debug Info Version", i32 3}
@@ -43,7 +44,7 @@ attributes #0 = { nofree nosync nounwind readnone speculatable willreturn }
4344
!11 = !DIFile(filename: "file1.st", directory: "app")
4445
!12 = !DISubroutineType(flags: DIFlagPublic, types: !13)
4546
!13 = !{null}
46-
!14 = !DILocalVariable(name: "main", scope: !10, file: !11, line: 2, type: !15)
47+
!14 = !DILocalVariable(name: "main", scope: !10, file: !11, line: 2, type: !15, align: 16)
4748
!15 = !DIBasicType(name: "INT", size: 16, encoding: DW_ATE_signed, flags: DIFlagPublic)
4849
!16 = !DILocation(line: 2, column: 13, scope: !10)
4950
!17 = !DILocation(line: 10, column: 4, scope: !10)
@@ -76,7 +77,7 @@ attributes #0 = { nofree nosync nounwind readnone speculatable willreturn }
7677
!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
7778
!1 = distinct !DIGlobalVariable(name: "mainProg", scope: !2, file: !2, line: 2, type: !3, isLocal: false, isDefinition: true)
7879
!2 = !DIFile(filename: "file2.st", directory: "lib")
79-
!3 = !DICompositeType(tag: DW_TAG_structure_type, name: "mainProg", scope: !2, file: !2, line: 2, flags: DIFlagPublic, elements: !4, identifier: "mainProg")
80+
!3 = !DICompositeType(tag: DW_TAG_structure_type, name: "mainProg", scope: !2, file: !2, line: 2, align: 64, flags: DIFlagPublic, elements: !4, identifier: "mainProg")
8081
!4 = !{}
8182
!5 = !{i32 2, !"Dwarf Version", i32 5}
8283
!6 = !{i32 2, !"Debug Info Version", i32 3}

compiler/plc_driver/src/tests/snapshots/plc_driver__tests__multi_files__multiple_files_with_debug_info.snap

+4-3
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
---
22
source: compiler/plc_driver/./src/tests/multi_files.rs
33
expression: "results.join(\"\\n\")"
4+
snapshot_kind: text
45
---
56
; ModuleID = 'file1.st'
67
source_filename = "file1.st"
@@ -32,7 +33,7 @@ attributes #0 = { nofree nosync nounwind readnone speculatable willreturn }
3233
!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
3334
!1 = distinct !DIGlobalVariable(name: "mainProg", scope: !2, file: !2, line: 2, type: !3, isLocal: false, isDefinition: true)
3435
!2 = !DIFile(filename: "file2.st", directory: "")
35-
!3 = !DICompositeType(tag: DW_TAG_structure_type, name: "mainProg", scope: !2, file: !2, line: 2, flags: DIFlagPublic, elements: !4, identifier: "mainProg")
36+
!3 = !DICompositeType(tag: DW_TAG_structure_type, name: "mainProg", scope: !2, file: !2, line: 2, align: 64, flags: DIFlagPublic, elements: !4, identifier: "mainProg")
3637
!4 = !{}
3738
!5 = !{i32 2, !"Dwarf Version", i32 5}
3839
!6 = !{i32 2, !"Debug Info Version", i32 3}
@@ -43,7 +44,7 @@ attributes #0 = { nofree nosync nounwind readnone speculatable willreturn }
4344
!11 = !DIFile(filename: "file1.st", directory: "")
4445
!12 = !DISubroutineType(flags: DIFlagPublic, types: !13)
4546
!13 = !{null}
46-
!14 = !DILocalVariable(name: "main", scope: !10, file: !11, line: 2, type: !15)
47+
!14 = !DILocalVariable(name: "main", scope: !10, file: !11, line: 2, type: !15, align: 16)
4748
!15 = !DIBasicType(name: "INT", size: 16, encoding: DW_ATE_signed, flags: DIFlagPublic)
4849
!16 = !DILocation(line: 2, column: 13, scope: !10)
4950
!17 = !DILocation(line: 10, column: 4, scope: !10)
@@ -76,7 +77,7 @@ attributes #0 = { nofree nosync nounwind readnone speculatable willreturn }
7677
!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
7778
!1 = distinct !DIGlobalVariable(name: "mainProg", scope: !2, file: !2, line: 2, type: !3, isLocal: false, isDefinition: true)
7879
!2 = !DIFile(filename: "file2.st", directory: "")
79-
!3 = !DICompositeType(tag: DW_TAG_structure_type, name: "mainProg", scope: !2, file: !2, line: 2, flags: DIFlagPublic, elements: !4, identifier: "mainProg")
80+
!3 = !DICompositeType(tag: DW_TAG_structure_type, name: "mainProg", scope: !2, file: !2, line: 2, align: 64, flags: DIFlagPublic, elements: !4, identifier: "mainProg")
8081
!4 = !{}
8182
!5 = !{i32 2, !"Dwarf Version", i32 5}
8283
!6 = !{i32 2, !"Debug Info Version", i32 3}

src/codegen/debug.rs

+111-33
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ pub trait Debug<'ink> {
7272
/// function's stub as well as its interface (variables/parameters)
7373
fn register_function<'idx>(
7474
&mut self,
75-
index: &Index,
75+
indices: (&Index, &LlvmTypedIndex<'ink>),
7676
func: &FunctionContext<'ink, 'idx>,
7777
return_type: Option<&'idx DataType>,
7878
parent_function: Option<FunctionValue<'ink>>,
@@ -303,44 +303,110 @@ impl<'ink> DebugBuilder<'ink> {
303303
.unwrap_or_else(|| self.compile_unit.get_file());
304304

305305
let mut types = vec![];
306-
for (element_index, (member_name, dt, location)) in index_types.into_iter().enumerate() {
306+
let mut last_offset_bits = 0;
307+
let mut last_size_bits = 0;
308+
309+
for (element_index, (member_name, dt, location)) in index_types.iter().enumerate() {
307310
let di_type = self.get_or_create_debug_type(dt, index, types_index)?;
308311

309-
//Adjust the offset based on the field alignment
310-
let size = types_index
311-
.find_associated_type(dt.get_name())
312-
.map(|llvm_type| self.target_data.get_bit_size(&llvm_type))
312+
// Get the size and alignment
313+
let llvm_type = types_index.find_associated_type(dt.get_name());
314+
let align_bits =
315+
llvm_type.map(|ty| self.target_data.get_preferred_alignment(&ty) * 8).unwrap_or(0);
316+
let size_bits = llvm_type.map(|ty| self.target_data.get_bit_size(&ty)).unwrap_or(0);
317+
318+
// Get LLVM's calculated offset
319+
let llvm_offset_bits = self
320+
.target_data
321+
.offset_of_element(&struct_type, element_index as u32)
322+
.map(|offset| offset * 8)
313323
.unwrap_or(0);
314-
//Offset in bits
315-
let offset =
316-
self.target_data.offset_of_element(&struct_type, element_index as u32).unwrap_or(0) * 8;
317324

325+
// Calculate the properly aligned offset based on the previous field
326+
// and this field's alignment requirements
327+
let offset_bits = if size_bits == 0 || (last_size_bits == 0 && element_index > 0) {
328+
// For zero-sized types, always use LLVM's offset directly
329+
// This ensures they don't contribute to the overall layout calculation
330+
// If the previous field was zero-sized, use LLVM's offset
331+
// for proper alignment of fields after zero-sized types
332+
llvm_offset_bits
333+
} else {
334+
let next_offset_bits: u64 = last_offset_bits + last_size_bits;
335+
336+
// Special handling based on alignment requirements:
337+
// - Fields with alignment of 64 bits need to be explicitly aligned
338+
// to their alignment boundary to prevent misaligned accesses
339+
// - Fields with a lower alignment can use LLVM's natural layout
340+
// This differentiation is crucial for correctly representing complex structures
341+
// where some fields (like LWORD, LINT) need specific alignment while others (like BYTE, BOOL)
342+
// can be packed more efficiently
343+
if align_bits == 64 {
344+
// For fields requiring special alignment (64 bit types),
345+
// round up to the nearest alignment boundary
346+
next_offset_bits.div_ceil(align_bits as u64) * align_bits as u64
347+
} else {
348+
// For smaller fields fields (BYTE, BOOL, DINT), we still ensure we don't
349+
// overlap with previous fields by using the maximum of our calculated offset
350+
// and LLVM's calculated offset
351+
std::cmp::max(next_offset_bits, llvm_offset_bits)
352+
}
353+
};
354+
355+
// Update tracking variables for next field
356+
last_offset_bits = offset_bits;
357+
last_size_bits = size_bits;
358+
359+
// Create the member type with calculated offset
318360
types.push(
319361
self.debug_info
320362
.create_member_type(
321363
file.as_debug_info_scope(),
322364
member_name,
323365
file,
324366
location.get_line_plus_one() as u32,
325-
size,
326-
0, // No set alignment
327-
offset,
367+
size_bits,
368+
align_bits,
369+
offset_bits,
328370
DIFlags::PUBLIC,
329371
di_type.into(),
330372
)
331373
.as_type(),
332374
);
333375
}
334376

335-
let size = self.target_data.get_bit_size(&struct_type);
336-
//Create a struct type
377+
// Calculate struct size based on the last field's offset + size, properly aligned
378+
let llvm_size = self.target_data.get_bit_size(&struct_type);
379+
// Calculate our manual size based on adjusted offsets
380+
let calculated_size = {
381+
// Get struct alignment requirement (usually 8 bytes/64 bits for 64-bit architectures)
382+
let struct_align_bits = self.target_data.get_preferred_alignment(&struct_type) * 8;
383+
384+
// Calculate total size based on last field offset + size, rounded up to alignment
385+
let last_field_end_bits = last_offset_bits + last_size_bits;
386+
let aligned_size_bits =
387+
last_field_end_bits.div_ceil(struct_align_bits as u64) * struct_align_bits as u64;
388+
389+
// If our calculated size is larger than LLVM's, use ours
390+
if aligned_size_bits > llvm_size {
391+
log::trace!(
392+
"Struct {}: adjusted size from {} to {} bits due to field alignment",
393+
name,
394+
llvm_size,
395+
aligned_size_bits
396+
);
397+
aligned_size_bits
398+
} else {
399+
llvm_size
400+
}
401+
};
402+
337403
let struct_type = self.debug_info.create_struct_type(
338404
file.as_debug_info_scope(),
339405
name,
340406
file,
341407
location.get_line_plus_one() as u32,
342-
size,
343-
0, // No set alignment
408+
calculated_size,
409+
self.target_data.get_preferred_alignment(&struct_type) * 8,
344410
DIFlags::PUBLIC,
345411
None,
346412
types.as_slice(),
@@ -364,20 +430,18 @@ impl<'ink> DebugBuilder<'ink> {
364430
) -> Result<(), Diagnostic> {
365431
//find the inner type debug info
366432
let inner_type = index.get_type(inner_type)?;
367-
//Find the dimenstions as ranges
433+
//Find the dimensions as ranges
368434
let subscript = dimensions
369435
.iter()
370436
.map(|it| it.get_range_plus_one(index))
371-
//Convert to normal range
372437
.collect::<Result<Vec<Range<i64>>, _>>()
373438
.map_err(|err| Diagnostic::codegen_error(err, SourceLocation::undefined()))?;
374439
let inner_type = self.get_or_create_debug_type(inner_type, index, types_index)?;
375-
let array_type = self.debug_info.create_array_type(
376-
inner_type.into(),
377-
size,
378-
0, //No set alignment
379-
subscript.as_slice(),
380-
);
440+
let llvm_type = types_index.get_associated_type(name)?;
441+
let align_bits = self.target_data.get_preferred_alignment(&llvm_type) * 8;
442+
let array_type =
443+
self.debug_info.create_array_type(inner_type.into(), size, align_bits, subscript.as_slice());
444+
381445
self.register_concrete_type(name, DebugType::Composite(array_type));
382446
Ok(())
383447
}
@@ -392,11 +456,13 @@ impl<'ink> DebugBuilder<'ink> {
392456
) -> Result<(), Diagnostic> {
393457
let inner_type = index.get_type(inner_type)?;
394458
let inner_type = self.get_or_create_debug_type(inner_type, index, types_index)?;
459+
let llvm_type = types_index.get_associated_type(name)?;
460+
let align_bits = self.target_data.get_preferred_alignment(&llvm_type) * 8;
395461
let pointer_type = self.debug_info.create_pointer_type(
396462
name,
397463
inner_type.into(),
398464
size,
399-
0, //No set alignment
465+
align_bits,
400466
inkwell::AddressSpace::from(ADDRESS_SPACE_GLOBAL),
401467
);
402468
self.register_concrete_type(name, DebugType::Derived(pointer_type));
@@ -438,14 +504,17 @@ impl<'ink> DebugBuilder<'ink> {
438504
StringEncoding::Utf16 => index.get_effective_type_or_void_by_name(WCHAR_TYPE),
439505
};
440506
let inner_type = self.get_or_create_debug_type(inner_type, index, types_index)?;
507+
let llvm_type = types_index.get_associated_type(name)?;
508+
let align_bits = self.target_data.get_preferred_alignment(&llvm_type) * 8;
441509
//Register an array
442510
let array_type = self.debug_info.create_array_type(
443511
inner_type.into(),
444512
size,
445-
0, //No set alignment
513+
align_bits,
446514
#[allow(clippy::single_range_in_vec_init)]
447515
&[(0..length)],
448516
);
517+
449518
self.register_concrete_type(name, DebugType::Composite(array_type));
450519
Ok(())
451520
}
@@ -465,13 +534,15 @@ impl<'ink> DebugBuilder<'ink> {
465534
.map(|it| self.get_or_create_debug_file(it))
466535
.unwrap_or_else(|| self.compile_unit.get_file());
467536

537+
let llvm_type = types_index.get_associated_type(name)?;
538+
let align_bits = self.target_data.get_preferred_alignment(&llvm_type) * 8;
468539
let typedef = self.debug_info.create_typedef(
469540
inner_type.into(),
470541
name,
471542
file,
472543
location.get_line_plus_one() as u32,
473544
file.as_debug_info_scope(),
474-
0, //No set alignment
545+
align_bits,
475546
);
476547
self.register_concrete_type(name, DebugType::Derived(typedef));
477548

@@ -547,6 +618,7 @@ impl<'ink> DebugBuilder<'ink> {
547618
pou: &PouIndexEntry,
548619
func: &FunctionContext<'ink, '_>,
549620
index: &Index,
621+
types_index: &LlvmTypedIndex<'ink>,
550622
) {
551623
let mut param_offset = 0;
552624
//Register the return and local variables for debugging
@@ -555,7 +627,12 @@ impl<'ink> DebugBuilder<'ink> {
555627
.iter()
556628
.filter(|it| it.is_local() || it.is_temp() || it.is_return())
557629
{
558-
self.register_local_variable(variable, 0, func);
630+
let align_bits = types_index
631+
.get_associated_type(&variable.data_type_name)
632+
.map(|it| self.target_data.get_preferred_alignment(&it))
633+
.unwrap_or(0)
634+
* 8;
635+
self.register_local_variable(variable, align_bits, func);
559636
}
560637

561638
let implementation = pou.find_implementation(index).expect("A POU will have an impl at this stage");
@@ -611,13 +688,14 @@ impl<'ink> Debug<'ink> for DebugBuilder<'ink> {
611688

612689
fn register_function<'idx>(
613690
&mut self,
614-
index: &Index,
691+
indices: (&Index, &LlvmTypedIndex<'ink>),
615692
func: &FunctionContext<'ink, 'idx>,
616693
return_type: Option<&'idx DataType>,
617694
parent_function: Option<FunctionValue<'ink>>,
618695
parameter_types: &[&'idx DataType],
619696
implementation_start: usize,
620697
) {
698+
let (index, types_index) = indices;
621699
let pou = index.find_pou(func.linking_context.get_call_name()).expect("POU is available");
622700
if matches!(pou.get_linkage(), LinkageType::External) || pou.get_location().is_internal() {
623701
return;
@@ -635,7 +713,7 @@ impl<'ink> Debug<'ink> for DebugBuilder<'ink> {
635713
let subprogram = self.create_function(scope, pou, return_type, parameter_types, implementation_start);
636714
func.function.set_subprogram(subprogram);
637715
//Create function parameters
638-
self.create_function_variables(pou, func, index);
716+
self.create_function_variables(pou, func, index, types_index);
639717
}
640718

641719
fn register_debug_type<'idx>(
@@ -720,7 +798,7 @@ impl<'ink> Debug<'ink> for DebugBuilder<'ink> {
720798
false,
721799
None,
722800
None,
723-
global_variable.get_alignment(),
801+
0, // Global variable alignment does not need to be forced/set. See https://llvm.org/docs/LangRef.html#global-variables
724802
);
725803
let gv_metadata = debug_variable.as_metadata_value(self.context);
726804

@@ -893,7 +971,7 @@ impl<'ink> Debug<'ink> for DebugBuilderEnum<'ink> {
893971

894972
fn register_function<'idx>(
895973
&mut self,
896-
index: &Index,
974+
indices: (&Index, &LlvmTypedIndex<'ink>),
897975
func: &FunctionContext<'ink, 'idx>,
898976
return_type: Option<&'idx DataType>,
899977
parent_function: Option<FunctionValue<'ink>>,
@@ -903,7 +981,7 @@ impl<'ink> Debug<'ink> for DebugBuilderEnum<'ink> {
903981
match self {
904982
Self::None | Self::VariablesOnly(..) => {}
905983
Self::Full(obj) => obj.register_function(
906-
index,
984+
indices,
907985
func,
908986
return_type,
909987
parent_function,

src/codegen/generators/pou_generator.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,7 @@ impl<'ink, 'cg> PouGenerator<'ink, 'cg> {
314314
blocks: FxHashMap::default(),
315315
};
316316
debug.register_function(
317-
self.index,
317+
(self.index, self.llvm_index),
318318
&function_context,
319319
return_type,
320320
parent_function,

0 commit comments

Comments
 (0)