[PATCH] D140111: [lld][wasm] Split __wasm_apply_data_relocs when it exceeds the function size limit
Sam Clegg via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 19 04:48:23 PST 2022
sbc100 added a comment.
I feel like a better approach might be to generate N different functions during the generation phase rather than trying to split the function after generation?
Also, I think we (also) want to investigate using a data driven approach for the relocations that don't involve symbol-specific globals. See the discussion on that here: https://github.com/emscripten-core/emscripten/issues/12682#issuecomment-1297755521
================
Comment at: lld/test/wasm/shared.s:1
-# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %s
-# RUN: wasm-ld --experimental-pic -shared -o %t.wasm %t.o
-# RUN: obj2yaml %t.wasm | FileCheck %s
-# RUN: llvm-objdump --disassemble-symbols=__wasm_call_ctors,__wasm_apply_data_relocs --no-show-raw-insn --no-leading-addr %t.wasm | FileCheck %s --check-prefixes DIS
-
-.functype func_external () -> ()
-
-# Linker-synthesized globals
-.globaltype __stack_pointer, i32
-.globaltype __table_base, i32, immutable
-.globaltype __memory_base, i32, immutable
-
-.section .data.data,"",@
-data:
- .p2align 2
- .int32 2
- .size data, 4
-
-.section .data.indirect_func_external,"",@
-indirect_func_external:
- .int32 func_external
-.size indirect_func_external, 4
-
-.section .data.indirect_func,"",@
-indirect_func:
- .int32 foo
- .size indirect_func, 4
-
-# Test data relocations
-
-.section .data.data_addr,"",@
-data_addr:
- .int32 data
- .size data_addr, 4
-
-# .. against external symbols
-
-.section .data.data_addr_external,"",@
-data_addr_external:
- .int32 data_external
- .size data_addr_external, 4
-
-# .. including addends
-
-.section .data.extern_struct_internal_ptr,"",@
-extern_struct_internal_ptr:
- .int32 extern_struct + 4
- .size extern_struct_internal_ptr, 4
-
-# Test use of __stack_pointer
-
-.section .text,"",@
-foo:
- # %ptr = alloca i32
- # %0 = load i32, ptr @data, align 4
- # %1 = load ptr, ptr @indirect_func, align 4
- # call i32 %1()
- # ret i32 %0
- .functype foo () -> (i32)
- .local i32, i32
- global.get __stack_pointer
- i32.const 16
- i32.sub
- local.tee 0
- global.set __stack_pointer
- global.get __memory_base
- i32.const data at MBREL
- i32.add
- i32.load 0
- local.set 1
- global.get indirect_func at GOT
- i32.load 0
- call_indirect () -> (i32)
- drop
- local.get 0
- i32.const 16
- i32.add
- global.set __stack_pointer
- local.get 1
- end_function
-
-get_func_address:
- .functype get_func_address () -> (i32)
- global.get func_external at GOT
- end_function
-
-get_data_address:
- .functype get_data_address () -> (i32)
- global.get data_external at GOT
- end_function
-
-get_local_func_address:
- # Verify that a function which is otherwise not address taken *is* added to
- # the wasm table with referenced via R_WASM_TABLE_INDEX_REL_SLEB
- .functype get_local_func_address () -> (i32)
- global.get __table_base
- i32.const get_func_address at TBREL
- i32.add
- end_function
-
-.globl foo
-.globl data
-.globl indirect_func
-.globl indirect_func_external
-.globl data_addr
-.globl data_addr_external
-.globl extern_struct_internal_ptr
-.globl get_data_address
-.globl get_func_address
-.globl get_local_func_address
-
-.hidden foo
-.hidden data
-.hidden get_data_address
-.hidden get_func_address
-
-# Without this linking will fail because we import __stack_pointer (a mutable
-# global).
-# TODO(sbc): We probably want a nicer way to specify target_features section
-# in assembly.
-.section .custom_section.target_features,"",@
-.int8 1
-.int8 43
-.int8 15
-.ascii "mutable-globals"
-
-# check for dylink section at start
-
-# CHECK: Sections:
-# CHECK-NEXT: - Type: CUSTOM
-# CHECK-NEXT: Name: dylink.0
-# CHECK-NEXT: MemorySize: 24
-# CHECK-NEXT: MemoryAlignment: 2
-# CHECK-NEXT: TableSize: 2
-# CHECK-NEXT: TableAlignment: 0
-# CHECK-NEXT: Needed: []
-# CHECK-NEXT: - Type: TYPE
-
-# check for import of __table_base and __memory_base globals
-
-# CHECK: - Type: IMPORT
-# CHECK-NEXT: Imports:
-# CHECK-NEXT: - Module: env
-# CHECK-NEXT: Field: memory
-# CHECK-NEXT: Kind: MEMORY
-# CHECK-NEXT: Memory:
-# CHECK-NEXT: Minimum: 0x1
-# CHECK-NEXT: - Module: env
-# CHECK-NEXT: Field: __indirect_function_table
-# CHECK-NEXT: Kind: TABLE
-# CHECK-NEXT: Table:
-# CHECK-NEXT: Index: 0
-# CHECK-NEXT: ElemType: FUNCREF
-# CHECK-NEXT: Limits:
-# CHECK-NEXT: Minimum: 0x2
-# CHECK-NEXT: - Module: env
-# CHECK-NEXT: Field: __stack_pointer
-# CHECK-NEXT: Kind: GLOBAL
-# CHECK-NEXT: GlobalType: I32
-# CHECK-NEXT: GlobalMutable: true
-# CHECK-NEXT: - Module: env
-# CHECK-NEXT: Field: __memory_base
-# CHECK-NEXT: Kind: GLOBAL
-# CHECK-NEXT: GlobalType: I32
-# CHECK-NEXT: GlobalMutable: false
-# CHECK-NEXT: - Module: env
-# CHECK-NEXT: Field: __table_base
-# CHECK-NEXT: Kind: GLOBAL
-# CHECK-NEXT: GlobalType: I32
-# CHECK-NEXT: GlobalMutable: false
-# CHECK-NEXT: - Module: GOT.mem
-# CHECK-NEXT: Field: indirect_func
-# CHECK-NEXT: Kind: GLOBAL
-# CHECK-NEXT: GlobalType: I32
-# CHECK-NEXT: GlobalMutable: true
-# CHECK-NEXT: - Module: GOT.func
-# CHECK-NEXT: Field: func_external
-# CHECK-NEXT: Kind: GLOBAL
-# CHECK-NEXT: GlobalType: I32
-# CHECK-NEXT: GlobalMutable: true
-# CHECK-NEXT: - Module: GOT.mem
-# CHECK-NEXT: Field: data_external
-# CHECK-NEXT: Kind: GLOBAL
-# CHECK-NEXT: GlobalType: I32
-# CHECK-NEXT: GlobalMutable: true
-# CHECK-NEXT: - Module: GOT.mem
-# CHECK-NEXT: Field: extern_struct
-# CHECK-NEXT: Kind: GLOBAL
-# CHECK-NEXT: GlobalType: I32
-# CHECK-NEXT: GlobalMutable: true
-# CHECK-NEXT: - Type: FUNCTION
-
-# CHECK: - Type: EXPORT
-# CHECK-NEXT: Exports:
-# CHECK-NEXT: - Name: __wasm_call_ctors
-# CHECK-NEXT: Kind: FUNCTION
-# CHECK-NEXT: Index: 0
-
-# check for elem segment initialized with __table_base global as offset
-
-# CHECK: - Type: ELEM
-# CHECK-NEXT: Segments:
-# CHECK-NEXT: - Offset:
-# CHECK-NEXT: Opcode: GLOBAL_GET
-# CHECK-NEXT: Index: 2
-# CHECK-NEXT: Functions: [ 3, 2 ]
-
-# check the generated code in __wasm_call_ctors and __wasm_apply_data_relocs functions
-
-# DIS: <__wasm_call_ctors>:
-# DIS-EMPTY:
-# DIS-NEXT: end
-
-# DIS: <__wasm_apply_data_relocs>:
-# DIS-EMPTY:
-# DIS-NEXT: i32.const 4
-# DIS-NEXT: global.get 1
-# DIS-NEXT: i32.add
-# DIS-NEXT: global.get 4
-# DIS-NEXT: i32.store 0
-# DIS-NEXT: i32.const 8
-# DIS-NEXT: global.get 1
-# DIS-NEXT: i32.add
-# DIS-NEXT: global.get 2
-# DIS-NEXT: i32.const 1
-# DIS-NEXT: i32.add
-# DIS-NEXT: i32.store 0
-# DIS-NEXT: i32.const 12
-# DIS-NEXT: global.get 1
-# DIS-NEXT: i32.add
-# DIS-NEXT: global.get 1
-# DIS-NEXT: i32.const 0
-# DIS-NEXT: i32.add
-# DIS-NEXT: i32.store 0
-# DIS-NEXT: i32.const 16
-# DIS-NEXT: global.get 1
-# DIS-NEXT: i32.add
-# DIS-NEXT: global.get 5
-# DIS-NEXT: i32.store 0
-# DIS-NEXT: i32.const 20
-# DIS-NEXT: global.get 1
-# DIS-NEXT: i32.add
-# DIS-NEXT: global.get 6
-# DIS-NEXT: i32.const 4
-# DIS-NEXT: i32.add
-# DIS-NEXT: i32.store 0
-# DIS-NEXT: end
-
-# check the data segment initialized with __memory_base global as offset
-
-# CHECK: - Type: DATA
-# CHECK-NEXT: Segments:
-# CHECK-NEXT: - SectionOffset: 6
-# CHECK-NEXT: InitFlags: 0
-# CHECK-NEXT: Offset:
-# CHECK-NEXT: Opcode: GLOBAL_GET
-# CHECK-NEXT: Index: 1
-# CHECK-NEXT: Content: '020000000000000001000000000000000000000000000000'
+# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %s
+# RUN: wasm-ld --experimental-pic -shared -o %t.wasm %t.o
----------------
Why are alll these test files showing up in the diff like every line of the file has changed? Did the line endings change maybe?
================
Comment at: lld/wasm/Driver.cpp:769
+ "__wasm_apply_data_relocs_tail",
+ WASM_SYMBOL_VISIBILITY_DEFAULT | WASM_SYMBOL_EXPORTED,
+ make<SyntheticFunction>(nullSignature,
----------------
We don't need either of these flags for the sub-function(s) I think
================
Comment at: lld/wasm/Driver.cpp:767
+
+ WasmSym::applyDataRelocsTail = symtab->addSyntheticFunction(
+ "__wasm_apply_data_relocs_tail",
----------------
k1nder10 wrote:
> I tried to move the creation of this function to Writer.cpp 1349 under 'else' statement, so that we don't have any changes in out .wasm files
> unless we exceed the function size limit. However, I got some strange runtime errors.
>
> So, for now, we will have this function created even If we don't hit the limit (it just will be empty). Is it okay?
I'd rather only emit the new function if needed.
Also, it seems like we should splitting into N possible sub-functions, not just 1, otherwise we just push the problem down the road a little.
================
Comment at: lld/wasm/SplitApplyDataRelocsStrategy.h:8
+
+class SplitApplyDataRelocsStrategy final : public SplitStrategy {
+public:
----------------
Creating a class hierarchy seems unnecessary since there is only a single sub-class. I'm not sure its worth adding new headers and source files either
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140111/new/
https://reviews.llvm.org/D140111
More information about the llvm-commits
mailing list