-
-
Notifications
You must be signed in to change notification settings - Fork 122
port(wasm): Resolve __data_end and __heap_base as linker data addresses
#2188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2844,6 +2844,7 @@ fn emit_reserved_linker_definitions(layout: &mut WasmLayout<'_>, indices: &Linke | |
| fn fill_linker_defined_values( | ||
| layout: &mut WasmLayout<'_>, | ||
| indices: &LinkerDefinedIndices, | ||
| needs_stack_gap: bool, | ||
| ) -> Result { | ||
| if let Some(defined_slot) = indices.stack_pointer_defined_slot { | ||
| let sp = layout | ||
|
|
@@ -2862,7 +2863,7 @@ fn fill_linker_defined_values( | |
| } | ||
|
|
||
| let mut bytes_needed = u64::from(layout.data_end.max(layout.memory_base)); | ||
| if indices.stack_pointer_defined_slot.is_some() { | ||
| if indices.stack_pointer_defined_slot.is_some() || needs_stack_gap { | ||
| bytes_needed = | ||
| bytes_needed.max(u64::from(layout.data_end.saturating_add(LINKER_STACK_SIZE))); | ||
| } | ||
|
|
@@ -3058,14 +3059,15 @@ where | |
| ensure_memory_export(&mut layout.exports); | ||
| } | ||
| layout.data_end = memory_cursor; | ||
| compute_data_addresses( | ||
| let needs_stack_gap = compute_data_addresses( | ||
| &mut layout.object_index_maps, | ||
| &layout.per_object_symbols, | ||
| &layout.object_data_layouts, | ||
| &layout_inputs, | ||
| symbol_db, | ||
| layout.data_end, | ||
| )?; | ||
| fill_linker_defined_values(&mut layout, &indices)?; | ||
| fill_linker_defined_values(&mut layout, &indices, needs_stack_gap)?; | ||
| ensure_entry_export( | ||
| &mut layout.exports, | ||
| &layout_inputs, | ||
|
|
@@ -3234,19 +3236,57 @@ fn data_symbol_memory_address( | |
| .ok_or_else(|| crate::error!("Wasm data symbol address overflow")) | ||
| } | ||
|
|
||
| /// Result of resolving a linker-defined data symbol name. | ||
| #[derive(Debug, Clone, Copy)] | ||
| struct LinkerDefinedDataAddress { | ||
| address: u32, | ||
| /// True when the address assumes a stack reservation after static data. | ||
| needs_stack_gap: bool, | ||
| } | ||
|
|
||
| /// Absolute addresses for linker-defined data symbols. | ||
| fn linker_defined_data_symbol_address( | ||
| name: &[u8], | ||
| data_end: u32, | ||
| ) -> Result<Option<LinkerDefinedDataAddress>> { | ||
| match name { | ||
| b"__data_end" => Ok(Some(LinkerDefinedDataAddress { | ||
| address: data_end, | ||
| needs_stack_gap: false, | ||
| })), | ||
| b"__heap_base" => { | ||
| let address = data_end | ||
| .checked_add(LINKER_STACK_SIZE) | ||
| .ok_or_else(|| crate::error!("Wasm __heap_base address overflow"))?; | ||
| Ok(Some(LinkerDefinedDataAddress { | ||
| address, | ||
| // We place the stack in [data_end, data_end + LINKER_STACK_SIZE) and define | ||
| // __heap_base as the end of that range (same as initial __stack_pointer). The | ||
| // address itself therefore requires that memory cover the stack gap. | ||
| needs_stack_gap: true, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is it about __heap_base that means it needs stack gap? Could be good to document that here.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I think I get it. I guess in the ELF parts of our linker, symbols like __heap_base would be attached to the start/end of sections, one of those sections would be the stack and memory addresses would just be computed by adding sizes.
lapla-cogito marked this conversation as resolved.
|
||
| })) | ||
| } | ||
| _ => Ok(None), | ||
| } | ||
| } | ||
|
|
||
| fn compute_data_addresses( | ||
| object_index_maps: &mut [WasmObjectIndexMap], | ||
| per_object_symbols: &[Vec<WasmSymbol>], | ||
| object_data_layouts: &[WasmObjectDataLayout<'_>], | ||
| layout_inputs: &[WasmObjectLayoutInput<'_>], | ||
| symbol_db: &SymbolDb<'_, Wasm>, | ||
| ) -> Result<()> { | ||
| data_end: u32, | ||
| ) -> Result<bool> { | ||
| let file_id_to_index: HashMap<crate::input_data::FileId, usize> = layout_inputs | ||
| .iter() | ||
| .enumerate() | ||
| .map(|(i, input)| (input.file_id, i)) | ||
| .collect(); | ||
|
|
||
| // True if any synthesised address assumes a stack gap after static data (`__heap_base`). | ||
| let mut needs_stack_gap = false; | ||
|
|
||
| for (obj_idx, (index_map, symbols)) in object_index_maps | ||
| .iter_mut() | ||
| .zip(per_object_symbols.iter()) | ||
|
|
@@ -3257,29 +3297,43 @@ fn compute_data_addresses( | |
| if sym.kind != WasmSymbolKind::Data { | ||
| continue; | ||
| } | ||
| let (def_obj_idx, def_sym) = if sym.is_undefined() { | ||
| let symbol_id = layout_inputs[obj_idx].symbol_id_range.offset_to_id(sym_idx); | ||
| let symbol_id = layout_inputs[obj_idx].symbol_id_range.offset_to_id(sym_idx); | ||
|
|
||
| // Resolve to the canonical definition when this is an undefined reference. | ||
| let (def_obj_idx, def_sym, name_symbol_id) = if sym.is_undefined() { | ||
| let def_id = symbol_db.definition(symbol_id); | ||
| if def_id == symbol_id { | ||
| continue; | ||
| } | ||
| let def_file_id = symbol_db.file_id_for_symbol(def_id); | ||
| let Some(&def_obj_idx) = file_id_to_index.get(&def_file_id) else { | ||
| continue; | ||
| }; | ||
| let def_input = &layout_inputs[def_obj_idx]; | ||
| let def_sym_offset = def_id.to_offset(def_input.symbol_id_range); | ||
| (def_obj_idx, per_object_symbols[def_obj_idx][def_sym_offset]) | ||
| ( | ||
| def_obj_idx, | ||
| per_object_symbols[def_obj_idx][def_sym_offset], | ||
| def_id, | ||
| ) | ||
| } else { | ||
| (obj_idx, *sym) | ||
| (obj_idx, *sym, symbol_id) | ||
| }; | ||
|
|
||
| if def_sym.is_undefined() { | ||
| let name = symbol_db.symbol_name(name_symbol_id)?; | ||
| if let Some(resolved) = linker_defined_data_symbol_address(name.bytes(), data_end)? | ||
| { | ||
| needs_stack_gap |= resolved.needs_stack_gap; | ||
| data_addresses[sym_idx] = resolved.address; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| data_addresses[sym_idx] = | ||
| data_symbol_memory_address(&object_data_layouts[def_obj_idx], &def_sym)?; | ||
| } | ||
| index_map.data_addresses = data_addresses; | ||
| } | ||
|
|
||
| Ok(()) | ||
| Ok(needs_stack_gap) | ||
| } | ||
|
|
||
| fn allocate_wasm_object_index_bases( | ||
|
|
@@ -4130,3 +4184,64 @@ fn wasm_symbol_from_info( | |
|
|
||
| sym | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn linker_defined_heap_base_requires_stack_gap() { | ||
| let data_end = 1024u32; | ||
| let de = linker_defined_data_symbol_address(b"__data_end", data_end) | ||
| .unwrap() | ||
| .expect("__data_end"); | ||
| assert_eq!(de.address, data_end); | ||
| assert!( | ||
| !de.needs_stack_gap, | ||
| "`__data_end` is the end of static data. It must not force a stack reservation" | ||
| ); | ||
|
|
||
| let hb = linker_defined_data_symbol_address(b"__heap_base", data_end) | ||
| .unwrap() | ||
| .expect("__heap_base"); | ||
| assert_eq!(hb.address, data_end + LINKER_STACK_SIZE); | ||
| assert!( | ||
| hb.needs_stack_gap, | ||
| "`__heap_base` sits past the stack gap, so memory must cover that range" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn needs_stack_gap_grows_memory_that_would_not_cover_heap_base() { | ||
| // Default freestanding links often already request 2 pages, which hides a missing | ||
| // needs_stack_gap. Therefore start from a 1-page memory so the grow path is observable. | ||
| let data_end = 1024u32; | ||
| let mut layout = WasmLayout { | ||
| data_end, | ||
| memories: vec![MemoryType { | ||
| memory64: false, | ||
| shared: false, | ||
| initial: 1, | ||
| maximum: None, | ||
| page_size_log2: None, | ||
| }], | ||
| ..Default::default() | ||
| }; | ||
| let indices = LinkerDefinedIndices::default(); | ||
|
|
||
| fill_linker_defined_values(&mut layout, &indices, false).unwrap(); | ||
| assert_eq!( | ||
| layout.memories[0].initial, 1, | ||
| "without `needs_stack_gap`, a 1-page memory must stay 1 page" | ||
| ); | ||
|
|
||
| fill_linker_defined_values(&mut layout, &indices, true).unwrap(); | ||
| let bytes_needed = u64::from(data_end) + u64::from(LINKER_STACK_SIZE); | ||
| let pages_needed = bytes_needed.div_ceil(65536); | ||
| assert_eq!( | ||
| layout.memories[0].initial, pages_needed, | ||
| "`needs_stack_gap` must grow memory to cover `data_end` + `LINKER_STACK_SIZE`" | ||
| ); | ||
| assert!(pages_needed > 1); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| //#Object:data-end-heap-base2.c | ||
|
|
||
| extern char __data_end; | ||
| extern char __heap_base; | ||
| extern int y; | ||
| extern unsigned long data_end_from_other_tu(void); | ||
| extern unsigned long heap_base_from_other_tu(void); | ||
|
|
||
| int x = 1; | ||
|
|
||
| void _start(void) { | ||
| unsigned long data_end = (unsigned long)&__data_end; | ||
| unsigned long heap_base = (unsigned long)&__heap_base; | ||
| unsigned long data_end2 = data_end_from_other_tu(); | ||
| unsigned long heap_base2 = heap_base_from_other_tu(); | ||
|
|
||
| // Static data starts above LINKER_MEMORY_BASE. exact end can differ by linker packing. | ||
| if (data_end <= 1024 || heap_base < data_end + 64 * 1024) { | ||
| __builtin_trap(); | ||
| } | ||
| if (data_end2 != data_end || heap_base2 != heap_base) { | ||
| __builtin_trap(); | ||
| } | ||
| if (x != 1 || y != 2) { | ||
| __builtin_trap(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| extern char __data_end; | ||
|
lapla-cogito marked this conversation as resolved.
|
||
| extern char __heap_base; | ||
|
|
||
| int y = 2; | ||
|
|
||
| unsigned long data_end_from_other_tu(void) { return (unsigned long)&__data_end; } | ||
| unsigned long heap_base_from_other_tu(void) { return (unsigned long)&__heap_base; } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to previous PRs, the matching on symbol names makes me wary. It feels potentially wasteful and like it ideally shouldn't be necessary. Fine for now, but longer-term it'd be good to consider alternatives.