wasm-ld: Implement function pointer alignment #105532

ethanaobrien · 2024-08-21T14:07:06Z

WIP implementation of function pointer alignment. Currently all values are statically declared.

padding: Waits x values before beginning the first function pointer. This can be useful for users needing to align on even numbers, or on odd numbers. This should possibly be equal to tableBase by default?
alignment: Inserts x-1 nullptr values into the function table.

Previous discussion: emscripten-core/emscripten#22369

github-actions · 2024-08-21T14:07:55Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-08-21T14:08:39Z

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-wasm

Author: Ethan O'Brien (ethanaobrien)

Changes

WIP implementation of function pointer alignment. Currently all values are statically declared.

padding: Waits x values before beginning the first function pointer. This can be useful for users needing to align on even numbers, or on odd numbers. This should possibly be equal to tableBase by default?
alignment: Inserts x-1 nullptr values into the function table.

Previous discussion: emscripten-core/emscripten#22369

Full diff: https://github.com/llvm/llvm-project/pull/105532.diff

1 Files Affected:

(modified) lld/wasm/SyntheticSections.cpp (+22)

diff --git a/lld/wasm/SyntheticSections.cpp b/lld/wasm/SyntheticSections.cpp
index f02f55519a2512..13333a67378681 100644
--- a/lld/wasm/SyntheticSections.cpp
+++ b/lld/wasm/SyntheticSections.cpp
@@ -563,8 +563,24 @@ void ElemSection::addEntry(FunctionSymbol *sym) {
   // They only exist so that the calls to missing functions can validate.
   if (sym->hasTableIndex() || sym->isStub)
     return;
+
+  uint32_t padding = 0;
+  uint64_t alignment = 1;
+
+  if (indirectFunctions.size() == 0 && padding > 0) {
+    for (uint32_t i=0; i<padding; i++) {
+      indirectFunctions.push_back(nullptr);
+    }
+  }
+
   sym->setTableIndex(config->tableBase + indirectFunctions.size());
   indirectFunctions.emplace_back(sym);
+
+  if (alignment > 1) {
+    for (uint32_t i=0; i<alignment-1; i++) {
+      indirectFunctions.push_back(nullptr);
+    }
+  }
 }
 
 void ElemSection::writeBody() {
@@ -602,6 +618,12 @@ void ElemSection::writeBody() {
   writeUleb128(os, indirectFunctions.size(), "elem count");
   uint32_t tableIndex = config->tableBase;
   for (const FunctionSymbol *sym : indirectFunctions) {
+    if (sym == nullptr) {
+      (void) tableIndex;
+      writeUleb128(os, 0, "function index");
+      ++tableIndex;
+      continue;
+    }
     assert(sym->getTableIndex() == tableIndex);
     (void) tableIndex;
     writeUleb128(os, sym->getFunctionIndex(), "function index");

ethanaobrien · 2024-12-29T21:50:03Z

@sbc100 Could I get some help cleaning this PR up and adding CLI arguments to influence these variables? I've tried a few times the last few months but don't seem to be familiar enough with the codebase to know how.

sbc100 · 2024-12-30T17:29:06Z

@sbc100 Could I get some help cleaning this PR up and adding CLI arguments to influence these variables? I've tried a few times the last few months but don't seem to be familiar enough with the codebase to know how.

To add new command line flags you would edit lld/wasm/Options.td to add your new flags and then lld/wasm/Config.h to add a place to store then update readConfigs in lld/wasm/Driver.cpp

ethanaobrien · 2024-12-30T17:40:57Z

Thanks! Before I do this, do you have opinions on what the flags should be?

From what I've been able to tell, to mimic how align-functions works on other targets, we would need to set padding to a static value of 1, and set alignment to the argument, though should the flag be align-functions? Should both padding and alignment be configurable?

sbc100 · 2024-12-30T17:45:49Z

Since we are implementing this as a linker flag and not a compiler flag I don't think there any need to mimik the existing compiler flags. I think simply --function-alignment=X is fine, or maybe even --function-pointer-alignment=X (since this only effects function pointer and not functions.

I don't think need a separate padding flag since we already have --table-base.

ethanaobrien · 2024-12-30T18:36:16Z

Thanks! This has been added, and is now ready for review.

Should this option also be added as an emscripten option? It seems it already was once upon a time. https://github.com/emscripten-core/emscripten/blob/b94d6e6ced33576a1ad96c7f6e701788c6f49211/src/settings.js#L2209C5-L2209C31

BinBashBanana · 2025-01-05T21:10:01Z

Merge this! :D

ethanaobrien · 2025-01-16T17:18:58Z

Hey @sbc100 is there any info on getting this reviewed? Thanks

BinBashBanana · 2025-01-22T02:21:13Z

bump

lld/wasm/SyntheticSections.cpp

sbc100 · 2025-01-22T02:33:54Z

Sorry I had some comments that were "Pending" since for a long time.

github-actions · 2025-01-22T02:41:09Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 9a95c097d0466c594f40a4ba9ced8a155574fdff 50aa21781adc97bad015d1fab5ad3415dfefcd58 --extensions cpp,h -- lld/wasm/Config.h lld/wasm/Driver.cpp lld/wasm/SyntheticSections.cpp

View the diff from clang-format here.

diff --git a/lld/wasm/Driver.cpp b/lld/wasm/Driver.cpp
index d8fd2b7820..4a0e923868 100644
--- a/lld/wasm/Driver.cpp
+++ b/lld/wasm/Driver.cpp
@@ -636,7 +636,8 @@ static void readConfigs(opt::InputArgList &args) {
   LLVM_DEBUG(errorHandler().verbose = true);
 
   ctx.arg.tableBase = args::getInteger(args, OPT_table_base, 0);
-  ctx.arg.functionPointerAlignment = args::getInteger(args, OPT_function_pointer_alignment, 0);
+  ctx.arg.functionPointerAlignment =
+      args::getInteger(args, OPT_function_pointer_alignment, 0);
   ctx.arg.globalBase = args::getInteger(args, OPT_global_base, 0);
   ctx.arg.initialHeap = args::getInteger(args, OPT_initial_heap, 0);
   ctx.arg.initialMemory = args::getInteger(args, OPT_initial_memory, 0);
diff --git a/lld/wasm/SyntheticSections.cpp b/lld/wasm/SyntheticSections.cpp
index 3fcef7cb3d..5ce0c5cbb4 100644
--- a/lld/wasm/SyntheticSections.cpp
+++ b/lld/wasm/SyntheticSections.cpp
@@ -570,7 +570,7 @@ void ElemSection::addEntry(FunctionSymbol *sym) {
   indirectFunctions.emplace_back(sym);
 
   if (ctx.arg.functionPointerAlignment > 1) {
-    for (uint32_t i=0; i<ctx.arg.functionPointerAlignment-1; i++) {
+    for (uint32_t i = 0; i < ctx.arg.functionPointerAlignment - 1; i++) {
       indirectFunctions.push_back(nullptr);
     }
   }
@@ -611,7 +611,7 @@ void ElemSection::writeBody() {
   uint32_t tableIndex = ctx.arg.tableBase;
   for (const FunctionSymbol *sym : indirectFunctions) {
     if (sym == nullptr) {
-      (void) tableIndex;
+      (void)tableIndex;
       writeUleb128(os, 0, "function index");
       ++tableIndex;
       continue;

ethanaobrien · 2025-01-22T04:13:03Z

All good @sbc100, I've also had that happen a few times too many 😄. This PR should be good to go now, with requested changes

ethanaobrien · 2025-02-21T03:16:38Z

Hey @sbc100, is there anything this is waiting on?

sbc100

Sorry for the delay. I think we still need a test for this new option. lgtm once we have a test.

ethanaobrien · 2025-02-28T13:44:09Z

May be a silly question - could you link where the emscripten tests are (so I can have something to base it off of and know where to put it). I'm unfamiliar with how this repo is organized

sbc100 · 2025-02-28T16:01:55Z

The wasm-ld tests live in lld/test/wasm. You can run them using llvm-lit. e.g. path/to/bin/llvm-lit lld/test/wasm/mytest.s (you can add -v and/or -a to get more info about what the test is doing.

ethanaobrien · 2025-05-12T03:57:38Z

Hey @sbc100! Sorry for the delay, I finally got the time to figure out how to write a test. I've added tests for default, an alignmnet of 2, and an alignment of 3

lld/test/wasm/function-pointer-alignmnet.s

lld/wasm/SyntheticSections.cpp

ethanaobrien · 2025-05-12T17:51:55Z

Er-wait, I'm now getting the null function or function signature mismatch when building with the first function at a different start index. This needs to be changed elsewhere too?

ethanaobrien · 2025-06-14T02:11:45Z

Hey @sbc100, do you have any advise on what other alignment I'm missing to get this working?

sbc100 · 2025-06-14T16:56:33Z

Hey @sbc100, do you have any advise on what other alignment I'm missing to get this working?

What exactly are you asking here? I looks like the current patch is causing a crash? I would try to reproduce it using a simple example, and build with debug info so you can see where the crash is happening.

If your example is simple enough you it should be possible so simply decompile it and see where the bad function pointer is.

ethanaobrien · 2025-06-14T16:59:18Z

I would try to reproduce it using a simple example, and build with debug info so you can see where the crash is happening.

It would appear adjusting the first function of the table to be aligned rather than at index zero causes the crash, likely something thinks the entry function isnt where it actually is from my findings?

sbc100 · 2025-06-14T17:15:28Z

I would try to reproduce it using a simple example, and build with debug info so you can see where the crash is happening.

It would appear adjusting the first function of the table to be aligned rather than at index zero causes the crash, likely something thinks the entry function isnt where it actually is from my findings?

It shouldn't matter where any given function is in the table, and there is nothing special about the any entry or first function (all functions pointers are just table indexes).

sbc100 · 2025-06-14T17:16:24Z

I would just build a simple example that makes a single indirect call to an empty function. It should be obivous from the disassembly if its address in the table is wrong.

ethanaobrien · 2025-06-14T17:43:14Z

It should be obivous from the disassembly if its address in the table is wrong.

It appears I was incorrect. You're right, main still fires how it should.

The issue is with apparently stdout somehow.

I have this test program a.c:

#include <stdio.h>
int main() {
   printf("Potato?\n");
   return 0;
}

Compiled with: EM_LLVM_ROOT=/home/ethan/git/llvm-project/build/bin emcc a.c -o a.html -Wl,--function-pointer-alignment=2 -gsource-map

And the error appears to be tied to the call to print (or specifically, this line)

Or similar happens if attempting to write data to a file:

https://github.com/emscripten-core/emscripten/blob/c38fe09642e25a02dce25ad3d760f4203940e527/system/lib/libc/musl/src/stdio/fflush.c#L29

What I dont understand is why changing the index of the first function broke this?

sbc100 · 2025-06-14T22:09:10Z

I would try with an even simpler program:

void foo() {
}
int main() {
  return &foo;   
}

You should be able to see the constant value that main returns and verify it is correct.

ethanaobrien · 2025-06-14T23:51:51Z

You should be able to see the constant value that main returns and verify it is correct.

Yep, this is correct

ethanaobrien · 2025-09-21T18:36:14Z

@sbc100 - I do want to try and finally get this finished and merged, though I don't know where to go. Things like writing to a file/stdout seem to be completely broken when changing the alignment of the first function and I don't know why or enough of this program to debug why. The small code snippet you shared involving no io works fine, though io seems completely broken. What's your opinion on this? Would always aligning at index 0 be an issue? I'm not sure what to do from here

ethanaobrien · 2025-10-20T00:00:01Z

I've rolled back to the last working change. It seems there's something else that doesnt like aligning the index of the first function, as per previous comments

sbc100 · 2025-10-20T15:18:19Z

I've rolled back to the last working change. It seems there's something else that doesnt like aligning the index of the first function, as per previous comments

That doesn't seems reasonable though does it? Having an option that aligns all function pointers except the first one seems very odd. I think we/you need to figure out what is going on a fix the issue such that all function pointers are aligned, not just some.

ethanaobrien · 2025-10-20T19:44:31Z

Yes, I agree. I'm just not sure what else to do and don't know enough how the codebase works to debug this deeper

It seems to happen when writing to stdout or writing a file, f->write seems to be an undefined or unaligned symbol.

https://github.com/emscripten-core/emscripten/blob/6ca370067cacb6a6102ed7245c611660ab38a514/system/lib/libc/musl/src/stdio/fwrite.c#L16

https://github.com/emscripten-core/emscripten/blob/6ca370067cacb6a6102ed7245c611660ab38a514/system/lib/libc/musl/src/stdio/fflush.c#L29C2-L29C15

ethanaobrien · 2025-10-21T15:17:43Z

I've created a minimal function to confirm this is something to do with the FILE->write addr. It makes no sense from my perspective why this would affect stdio functions at all, which is why I have no idea where to even begin to debug this.

int get_write_addr(FILE *f)
{
        f->write(f, 0, 0);
        return (int)&f->write;
}

sbc100 · 2025-10-21T15:41:31Z

Can you also share the main function you uses in the reproducer.

Almost certainly this is related to relocations since I believe the function pointer that are part of the standard streams are setup statically which means writing relocations to the data section that link time. See these three lines: https://github.com/emscripten-core/emscripten/blob/9110edd6366ae9f40fb91b1317f18d41df9df317/system/lib/libc/musl/src/stdio/stderr.c#L12-L14

My guess is you should be able to reproduce without stdio with a simple static data structure like the one listed above.

It should be fairly easy to confirm statically using a small example if the linker has written the correct values (for functions pointers) into the data section.

ethanaobrien · 2025-10-21T17:08:52Z

You are correct, I've created a new minimal example:

int foo(void) {
    return 3;
}

struct test {
    int fd;
    int (*write) (void);
};
struct test testing = {
	.fd = 2,
	.write = foo,
};

int main(int argc, char **argv) {
    if (argc == 1) return (int)&testing.write;
    if (argc == 2) return (int)&foo ;
    if (argc == 3) return foo();
    else return testing.write();
}

For reference, my old main:

int main() {
    FILE *f;
    f = fopen("test", "w");
    return get_write_addr(f);
}

I'm not too sure where I need to go from here

Implement wip WebAssembly function alignment

4ce4fe2

ethanaobrien marked this pull request as draft August 21, 2024 14:07

llvmbot added lld lld:wasm labels Aug 21, 2024

Merge branch 'llvm:main' into main

58d4069

ethanaobrien mentioned this pull request Dec 29, 2024

32X emulation - emscripten libretro/picodrive#233

Closed

[wasm] Add function-pointer-alignment function

d8df549

ethanaobrien changed the title ~~[WIP] wasm-ld: Implement function pointer alignment~~ wasm-ld: Implement function pointer alignment Dec 30, 2024

ethanaobrien marked this pull request as ready for review December 30, 2024 18:35

ethanaobrien mentioned this pull request Jan 2, 2025

Broken picodrive core EmulatorJS/EmulatorJS#579

Closed

Merge branch 'main' into main

50aa217

sbc100 reviewed Jan 22, 2025

View reviewed changes

lld/wasm/SyntheticSections.cpp Outdated Show resolved Hide resolved

lld/wasm/SyntheticSections.cpp Outdated Show resolved Hide resolved

Requested changes

983433d

Correct indentation

bbaf4e4

ethanaobrien requested a review from sbc100 January 23, 2025 17:44

sbc100 reviewed Feb 25, 2025

View reviewed changes

Add tests

a10be02

ethanaobrien force-pushed the main branch from fcc6899 to a10be02 Compare May 12, 2025 03:58

Simplify test

391f436

sbc100 reviewed May 12, 2025

View reviewed changes

lld/test/wasm/function-pointer-alignmnet.s Show resolved Hide resolved

sbc100 reviewed May 12, 2025

View reviewed changes

lld/wasm/SyntheticSections.cpp Outdated Show resolved Hide resolved

sbc100 reviewed May 12, 2025

View reviewed changes

lld/wasm/SyntheticSections.cpp Show resolved Hide resolved

Requested changes

ec254cd

ethanaobrien added 2 commits October 19, 2025 18:31

Merge remote-tracking branch 'upstream'

bdc2275

Revert to alignment 0 for first function

5830aec

ethanaobrien requested a review from sbc100 October 20, 2025 00:00

wasm-ld: Implement function pointer alignment #105532

Are you sure you want to change the base?

wasm-ld: Implement function pointer alignment #105532

Conversation

ethanaobrien commented Aug 21, 2024

Uh oh!

github-actions bot commented Aug 21, 2024

Uh oh!

llvmbot commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ethanaobrien commented Dec 29, 2024

Uh oh!

sbc100 commented Dec 30, 2024

Uh oh!

ethanaobrien commented Dec 30, 2024

Uh oh!

sbc100 commented Dec 30, 2024

Uh oh!

ethanaobrien commented Dec 30, 2024

Uh oh!

BinBashBanana commented Jan 5, 2025

Uh oh!

ethanaobrien commented Jan 16, 2025

Uh oh!

BinBashBanana commented Jan 22, 2025

Uh oh!

Uh oh!

Uh oh!

sbc100 commented Jan 22, 2025

Uh oh!

github-actions bot commented Jan 22, 2025

Uh oh!

ethanaobrien commented Jan 22, 2025

Uh oh!

ethanaobrien commented Feb 21, 2025

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

ethanaobrien commented Feb 28, 2025

Uh oh!

sbc100 commented Feb 28, 2025

Uh oh!

ethanaobrien commented May 12, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ethanaobrien commented May 12, 2025

Uh oh!

ethanaobrien commented Jun 14, 2025

Uh oh!

sbc100 commented Jun 14, 2025

Uh oh!

ethanaobrien commented Jun 14, 2025

Uh oh!

sbc100 commented Jun 14, 2025

Uh oh!

sbc100 commented Jun 14, 2025

Uh oh!

ethanaobrien commented Jun 14, 2025

Uh oh!

sbc100 commented Jun 14, 2025

Uh oh!

ethanaobrien commented Jun 14, 2025

Uh oh!

ethanaobrien commented Sep 21, 2025

Uh oh!

ethanaobrien commented Oct 20, 2025

Uh oh!

sbc100 commented Oct 20, 2025

Uh oh!

ethanaobrien commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ethanaobrien commented Oct 21, 2025

Uh oh!

sbc100 commented Oct 21, 2025

Uh oh!

llvmbot commented Aug 21, 2024 •

edited

Loading

ethanaobrien commented Oct 20, 2025 •

edited

Loading

ethanaobrien commented Oct 21, 2025 •

edited

Loading