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.

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-wasm

Author: Ethan O'Brien (ethanaobrien)

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");

@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 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

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?

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.

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

Merge this! :D

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

bump

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

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

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

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;

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

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

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

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.

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

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?