[lld][WebAssembly] Report unsupported PIC relocations as errors #104926

luc-blaeser · 2024-08-20T13:03:53Z

WASM_MEMORY_ADDR_REL_ and WASM_TABLE_INDEX_REL_ relocations against undefined symbols are not supported and, except for UnresolvedPolicy::ReportError, lead to incorrect Wasm code, such as invalid data address or invalid table index that cannot be patched during later dynamic Wasm linking with modules declaring those symbols. This is different to other relocations that support undefined symbols by declaring correspond Wasm imports.

For more robust behavior, wasm-ld should probably report an error for such unsupported PIC relocations, independent of the UnresolvedPolicy.

github-actions · 2024-08-20T13:04:17Z

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-20T15:05:08Z

@llvm/pr-subscribers-lld-wasm

Author: Luc Blaeser (luc-blaeser)

Changes

WASM_MEMORY_ADDR_REL_ and WASM_TABLE_INDEX_REL_ relocations against undefined symbols are not supported and, except for UnresolvedPolicy::ReportError, lead to incorrect Wasm code, such as invalid data address or invalid table index that cannot be patched during later dynamic Wasm linking with modules declaring those symbols. This is different to other relocations that support undefined symbols by declaring correspond Wasm imports.

For more robust behavior, wasm-ld should probably report an error for such unsupported PIC relocations, independent of the UnresolvedPolicy.

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

3 Files Affected:

(added) lld/test/wasm/unsupported-pic-relocations.s (+47)
(added) lld/test/wasm/unsupported-pic-relocations64.s (+48)
(modified) lld/wasm/Relocations.cpp (+13)

diff --git a/lld/test/wasm/unsupported-pic-relocations.s b/lld/test/wasm/unsupported-pic-relocations.s
new file mode 100644
index 00000000000000..81b9fdde83e677
--- /dev/null
+++ b/lld/test/wasm/unsupported-pic-relocations.s
@@ -0,0 +1,47 @@
+# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %s
+
+# RUN: not wasm-ld --experimental-pic -shared %t.o -o /dev/null 2>&1 | \
+# RUN:   FileCheck %s
+
+# RUN: not wasm-ld --experimental-pic -shared %t.o -o /dev/null  --unresolved-symbols=report-all 2>&1 | \
+# RUN:   FileCheck %s
+
+# RUN: not wasm-ld --experimental-pic -shared %t.o -o /dev/null  --warn-unresolved-symbols 2>&1 | \
+# RUN:   FileCheck %s
+
+# RUN: not wasm-ld --experimental-pic -shared %t.o -o /dev/null  --unresolved-symbols=ignore-all 2>&1 | \
+# RUN:   FileCheck %s
+
+# RUN: not wasm-ld --experimental-pic -shared %t.o -o /dev/null  --unresolved-symbols=import-dynamic 2>&1 | \
+# RUN:   FileCheck %s
+
+.globaltype __memory_base, i32, immutable
+.globaltype	__table_base, i32, immutable
+
+.functype external_func () -> ()
+
+call_undefined_function:
+    .functype call_undefined_function () -> ()
+    global.get  __table_base
+    i32.const external_func@TBREL
+    # CHECK: error: {{.*}}.o: relocation R_WASM_TABLE_INDEX_REL_SLEB is not supported against an undefined symbol `external_func`
+    i32.add
+    call_indirect () -> ()
+    end_function
+    
+access_undefined_data:
+    .functype access_undefined_data () -> ()
+    global.get  __memory_base
+    i32.const external_data@MBREL
+    # CHECK: error: {{.*}}.o: relocation R_WASM_MEMORY_ADDR_REL_SLEB is not supported against an undefined symbol `external_data`
+    i32.add
+    i32.load 0
+    drop
+    end_function
+
+.globl _start
+_start:
+    .functype _start () -> ()
+    call call_undefined_function
+    call access_undefined_data
+    end_function
diff --git a/lld/test/wasm/unsupported-pic-relocations64.s b/lld/test/wasm/unsupported-pic-relocations64.s
new file mode 100644
index 00000000000000..cae78d95862b48
--- /dev/null
+++ b/lld/test/wasm/unsupported-pic-relocations64.s
@@ -0,0 +1,48 @@
+# RUN: llvm-mc -filetype=obj -triple=wasm64-unknown-unknown -o %t.o %s
+
+# RUN: not wasm-ld -mwasm64 --experimental-pic -shared %t.o -o /dev/null 2>&1 | \
+# RUN:   FileCheck %s
+
+# RUN: not wasm-ld -mwasm64 --experimental-pic -shared %t.o -o /dev/null  --unresolved-symbols=report-all 2>&1 | \
+# RUN:   FileCheck %s
+
+# RUN: not wasm-ld -mwasm64 --experimental-pic -shared %t.o -o /dev/null  --warn-unresolved-symbols 2>&1 | \
+# RUN:   FileCheck %s
+
+# RUN: not wasm-ld -mwasm64 --experimental-pic -shared %t.o -o /dev/null  --unresolved-symbols=ignore-all 2>&1 | \
+# RUN:   FileCheck %s
+
+# RUN: not wasm-ld -mwasm64 --experimental-pic -shared %t.o -o /dev/null  --unresolved-symbols=import-dynamic 2>&1 | \
+# RUN:   FileCheck %s
+
+.globaltype __memory_base, i64, immutable
+.globaltype	__table_base, i64, immutable
+
+.functype external_func () -> ()
+
+call_undefined_function:
+    .functype call_undefined_function () -> ()
+    global.get  __table_base
+    i64.const external_func@TBREL
+    # CHECK: error: {{.*}}.o: relocation R_WASM_TABLE_INDEX_REL_SLEB64 is not supported against an undefined symbol `external_func`
+    i64.add
+    i32.wrap_i64 # Remove when table64 is supported
+    call_indirect () -> ()
+    end_function
+    
+access_undefined_data:
+    .functype access_undefined_data () -> ()
+    global.get  __memory_base
+    i64.const external_data@MBREL
+    # CHECK: error: {{.*}}.o: relocation R_WASM_MEMORY_ADDR_REL_SLEB64 is not supported against an undefined symbol `external_data`
+    i64.add
+    i64.load 0
+    drop
+    end_function
+
+.globl _start
+_start:
+    .functype _start () -> ()
+    call call_undefined_function
+    call access_undefined_data
+    end_function
diff --git a/lld/wasm/Relocations.cpp b/lld/wasm/Relocations.cpp
index 6f33a4f28a9d09..6b177f4f34bb6f 100644
--- a/lld/wasm/Relocations.cpp
+++ b/lld/wasm/Relocations.cpp
@@ -170,6 +170,19 @@ void scanRelocations(InputChunk *chunk) {
         if (requiresGOTAccess(sym))
           addGOTEntry(sym);
         break;
+      case R_WASM_TABLE_INDEX_REL_SLEB:
+      case R_WASM_TABLE_INDEX_REL_SLEB64:
+      case R_WASM_MEMORY_ADDR_REL_SLEB:
+      case R_WASM_MEMORY_ADDR_REL_SLEB64:
+        // These relocation types are only present in the code section and
+        // are not supported. It would require replacing the constant by using
+        // a GOT global.
+        if (sym->isUndefined())
+          error(toString(file) + ": relocation " +
+                relocTypeToString(reloc.Type) +
+                " is not supported against an undefined symbol `" +
+                toString(*sym) + "`");
+        break;
       }
     }

llvmbot · 2024-08-20T15:05:09Z

@llvm/pr-subscribers-lld

Author: Luc Blaeser (luc-blaeser)

Changes

WASM_MEMORY_ADDR_REL_ and WASM_TABLE_INDEX_REL_ relocations against undefined symbols are not supported and, except for UnresolvedPolicy::ReportError, lead to incorrect Wasm code, such as invalid data address or invalid table index that cannot be patched during later dynamic Wasm linking with modules declaring those symbols. This is different to other relocations that support undefined symbols by declaring correspond Wasm imports.

For more robust behavior, wasm-ld should probably report an error for such unsupported PIC relocations, independent of the UnresolvedPolicy.

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

3 Files Affected:

(added) lld/test/wasm/unsupported-pic-relocations.s (+47)
(added) lld/test/wasm/unsupported-pic-relocations64.s (+48)
(modified) lld/wasm/Relocations.cpp (+13)

diff --git a/lld/test/wasm/unsupported-pic-relocations.s b/lld/test/wasm/unsupported-pic-relocations.s
new file mode 100644
index 00000000000000..81b9fdde83e677
--- /dev/null
+++ b/lld/test/wasm/unsupported-pic-relocations.s
@@ -0,0 +1,47 @@
+# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %s
+
+# RUN: not wasm-ld --experimental-pic -shared %t.o -o /dev/null 2>&1 | \
+# RUN:   FileCheck %s
+
+# RUN: not wasm-ld --experimental-pic -shared %t.o -o /dev/null  --unresolved-symbols=report-all 2>&1 | \
+# RUN:   FileCheck %s
+
+# RUN: not wasm-ld --experimental-pic -shared %t.o -o /dev/null  --warn-unresolved-symbols 2>&1 | \
+# RUN:   FileCheck %s
+
+# RUN: not wasm-ld --experimental-pic -shared %t.o -o /dev/null  --unresolved-symbols=ignore-all 2>&1 | \
+# RUN:   FileCheck %s
+
+# RUN: not wasm-ld --experimental-pic -shared %t.o -o /dev/null  --unresolved-symbols=import-dynamic 2>&1 | \
+# RUN:   FileCheck %s
+
+.globaltype __memory_base, i32, immutable
+.globaltype	__table_base, i32, immutable
+
+.functype external_func () -> ()
+
+call_undefined_function:
+    .functype call_undefined_function () -> ()
+    global.get  __table_base
+    i32.const external_func@TBREL
+    # CHECK: error: {{.*}}.o: relocation R_WASM_TABLE_INDEX_REL_SLEB is not supported against an undefined symbol `external_func`
+    i32.add
+    call_indirect () -> ()
+    end_function
+    
+access_undefined_data:
+    .functype access_undefined_data () -> ()
+    global.get  __memory_base
+    i32.const external_data@MBREL
+    # CHECK: error: {{.*}}.o: relocation R_WASM_MEMORY_ADDR_REL_SLEB is not supported against an undefined symbol `external_data`
+    i32.add
+    i32.load 0
+    drop
+    end_function
+
+.globl _start
+_start:
+    .functype _start () -> ()
+    call call_undefined_function
+    call access_undefined_data
+    end_function
diff --git a/lld/test/wasm/unsupported-pic-relocations64.s b/lld/test/wasm/unsupported-pic-relocations64.s
new file mode 100644
index 00000000000000..cae78d95862b48
--- /dev/null
+++ b/lld/test/wasm/unsupported-pic-relocations64.s
@@ -0,0 +1,48 @@
+# RUN: llvm-mc -filetype=obj -triple=wasm64-unknown-unknown -o %t.o %s
+
+# RUN: not wasm-ld -mwasm64 --experimental-pic -shared %t.o -o /dev/null 2>&1 | \
+# RUN:   FileCheck %s
+
+# RUN: not wasm-ld -mwasm64 --experimental-pic -shared %t.o -o /dev/null  --unresolved-symbols=report-all 2>&1 | \
+# RUN:   FileCheck %s
+
+# RUN: not wasm-ld -mwasm64 --experimental-pic -shared %t.o -o /dev/null  --warn-unresolved-symbols 2>&1 | \
+# RUN:   FileCheck %s
+
+# RUN: not wasm-ld -mwasm64 --experimental-pic -shared %t.o -o /dev/null  --unresolved-symbols=ignore-all 2>&1 | \
+# RUN:   FileCheck %s
+
+# RUN: not wasm-ld -mwasm64 --experimental-pic -shared %t.o -o /dev/null  --unresolved-symbols=import-dynamic 2>&1 | \
+# RUN:   FileCheck %s
+
+.globaltype __memory_base, i64, immutable
+.globaltype	__table_base, i64, immutable
+
+.functype external_func () -> ()
+
+call_undefined_function:
+    .functype call_undefined_function () -> ()
+    global.get  __table_base
+    i64.const external_func@TBREL
+    # CHECK: error: {{.*}}.o: relocation R_WASM_TABLE_INDEX_REL_SLEB64 is not supported against an undefined symbol `external_func`
+    i64.add
+    i32.wrap_i64 # Remove when table64 is supported
+    call_indirect () -> ()
+    end_function
+    
+access_undefined_data:
+    .functype access_undefined_data () -> ()
+    global.get  __memory_base
+    i64.const external_data@MBREL
+    # CHECK: error: {{.*}}.o: relocation R_WASM_MEMORY_ADDR_REL_SLEB64 is not supported against an undefined symbol `external_data`
+    i64.add
+    i64.load 0
+    drop
+    end_function
+
+.globl _start
+_start:
+    .functype _start () -> ()
+    call call_undefined_function
+    call access_undefined_data
+    end_function
diff --git a/lld/wasm/Relocations.cpp b/lld/wasm/Relocations.cpp
index 6f33a4f28a9d09..6b177f4f34bb6f 100644
--- a/lld/wasm/Relocations.cpp
+++ b/lld/wasm/Relocations.cpp
@@ -170,6 +170,19 @@ void scanRelocations(InputChunk *chunk) {
         if (requiresGOTAccess(sym))
           addGOTEntry(sym);
         break;
+      case R_WASM_TABLE_INDEX_REL_SLEB:
+      case R_WASM_TABLE_INDEX_REL_SLEB64:
+      case R_WASM_MEMORY_ADDR_REL_SLEB:
+      case R_WASM_MEMORY_ADDR_REL_SLEB64:
+        // These relocation types are only present in the code section and
+        // are not supported. It would require replacing the constant by using
+        // a GOT global.
+        if (sym->isUndefined())
+          error(toString(file) + ": relocation " +
+                relocTypeToString(reloc.Type) +
+                " is not supported against an undefined symbol `" +
+                toString(*sym) + "`");
+        break;
       }
     }

lld/wasm/Relocations.cpp

sbc100

lgtm % some test cleanups. I think we can maybe make the tests much smaller.

lld/test/wasm/unsupported-pic-relocations.s

lld/wasm/Relocations.cpp

luc-blaeser · 2024-09-16T12:26:42Z

Ping

dschuff · 2024-09-16T17:27:22Z

I don't see another revision after you responded to the comments. Did you forget to push it?

luc-blaeser · 2024-09-16T17:35:00Z

I don't see another revision after you responded to the comments. Did you forget to push it?

Sorry, my mistake, I forgot to push.

dschuff · 2024-09-16T18:16:43Z

Thanks! Do need me to commit this?

lld/test/wasm/unsupported-pic-relocations.s

luc-blaeser · 2024-09-18T06:39:48Z

Thanks! Do need me to commit this?

Thank you also for your time! Please feel free to merge when you think it is okay. I believe I have no right to merge.

github-actions · 2024-09-18T15:40:49Z

@luc-blaeser Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

dschuff · 2024-09-18T15:41:16Z

Thanks for the contribution!

…#104926) `WASM_MEMORY_ADDR_REL_` and `WASM_TABLE_INDEX_REL_` relocations against **undefined symbols** are not supported and, except for `UnresolvedPolicy::ReportError`, lead to incorrect Wasm code, such as invalid data address or invalid table index that cannot be patched during later dynamic Wasm linking with modules declaring those symbols. This is different to other relocations that support undefined symbols by declaring correspond Wasm imports. For more robust behavior, `wasm-ld` should probably report an error for such unsupported PIC relocations, independent of the `UnresolvedPolicy`.

dschuff · 2024-09-20T21:21:58Z

This seems to have broken the Emscripten benchmark test test_zzz_bullet. (Build log here)

The command line is /b/s/w/ir/x/w/install/bin/wasm-ld -o .libs/libLinearMath.so.0.0.0 .libs/btQuickprof.o .libs/btGeometryUtil.o .libs/btAlignedAllocator.o .libs/btSerializer.o .libs/btConvexHull.o .libs/btConvexHullComputer.o -L/b/s/w/ir/x/w/install/emscripten/cache/sysroot/lib/wasm32-emscripten /b/s/w/ir/x/w/install/emscripten/cache/sysroot/lib/wasm32-emscripten/libGL-getprocaddr.a -lal -lhtml5 -lstubs-debug -lc-debug /b/s/w/ir/x/w/install/emscripten/cache/sysroot/lib/wasm32-emscripten/libdlmalloc.a -lcompiler_rt /b/s/w/ir/x/w/install/emscripten/cache/sysroot/lib/wasm32-emscripten/libc++-noexcept.a /b/s/w/ir/x/w/install/emscripten/cache/sysroot/lib/wasm32-emscripten/libc++abi-noexcept.a -lsockets -L/b/s/w/ir/x/w/install/emscripten/cache/sysroot/lib/wasm32-emscripten --relocatable -mllvm -combiner-global-alias-analysis=false -mllvm -enable-emscripten-sjlj -mllvm -disable-lsr

It looks like this command is trying to build a DSO but I also see it's using --relocatable and not -shared.
@sbc100 is this what that test is supposed to do? Maybe the test is broken somehow?

There are also warnings like
em++: warning: linking a library with -sharedwill emit a static object file. This is a form of emulation to support existing build systems. If you want to build a runtime shared library use the SIDE_MODULE setting. [-Wemcc] em++: warning: ignoring unsupported linker flag:-soname [-Wlinkflags]

which also seems maybe wrong.

sbc100 · 2024-09-20T21:50:08Z

Interesting.. all those errors are about __dso_handle, which by definition is always available at static link time.

I think the issue here is that we probably don't want to be reporting these errors in --relocatable mode (i.e. when we are just making another object file).

sbc100 · 2024-09-24T15:58:50Z

Fix is in #109822

…/--relocatable Followup to llvm#104926. We ran into issues on the emscripten waterfall where relocation against `__dso_handle` were being reported as errors even though `-r/--relocatable` was being used to generate object file output rather than executable output.

…/--relocatable (#109822) Followup to #104926. We ran into issues on the emscripten waterfall where relocation against `__dso_handle` were being reported as errors even though `-r/--relocatable` was being used to generate object file output rather than executable output.

Report unsupported PIC relocations in code segment

020ba6c

luc-blaeser changed the title ~~[lld][WebAssembly] Report Unsupported PIC Relocations in Code Segment~~ [lld][WebAssembly] Report Unsupported PIC Relocations as Errors Aug 20, 2024

luc-blaeser mentioned this pull request Aug 20, 2024

[lld][WebAssembly] Fix relocation of Wasm table index with GOT access #104043

Open

luc-blaeser marked this pull request as ready for review August 20, 2024 15:04

llvmbot added lld lld:wasm labels Aug 20, 2024

sbc100 reviewed Aug 21, 2024

View reviewed changes

lld/wasm/Relocations.cpp Show resolved Hide resolved

Adjust condition for unsupported PIC relocations

9684416

sbc100 reviewed Aug 21, 2024

View reviewed changes

lld/test/wasm/unsupported-pic-relocations.s Outdated Show resolved Hide resolved

sbc100 reviewed Aug 21, 2024

View reviewed changes

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

Code and test refactoring

4a66aaf

dschuff approved these changes Sep 16, 2024

View reviewed changes

sbc100 reviewed Sep 17, 2024

View reviewed changes

lld/test/wasm/unsupported-pic-relocations.s Show resolved Hide resolved

sbc100 approved these changes Sep 18, 2024

View reviewed changes

sbc100 changed the title ~~[lld][WebAssembly] Report Unsupported PIC Relocations as Errors~~ [lld][WebAssembly] Report unsupported PIC relocations as errors Sep 18, 2024

dschuff merged commit 6ce1409 into llvm:main Sep 18, 2024
8 checks passed

luc-blaeser deleted the luc/unsupported-pic-relocations branch September 18, 2024 16:39

sbc100 mentioned this pull request Sep 24, 2024

[lld][WebAssembly] Don't report relocation error when linking with -r/--relocatable #109822

Merged

[lld][WebAssembly] Report unsupported PIC relocations as errors #104926

[lld][WebAssembly] Report unsupported PIC relocations as errors #104926

Uh oh!

Conversation

luc-blaeser commented Aug 20, 2024

Uh oh!

github-actions bot commented Aug 20, 2024

Uh oh!

llvmbot commented Aug 20, 2024

Uh oh!

llvmbot commented Aug 20, 2024

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

luc-blaeser commented Sep 16, 2024

Uh oh!

dschuff commented Sep 16, 2024

Uh oh!

luc-blaeser commented Sep 16, 2024

Uh oh!

dschuff commented Sep 16, 2024

Uh oh!

Uh oh!

luc-blaeser commented Sep 18, 2024

Uh oh!

Uh oh!

github-actions bot commented Sep 18, 2024

Uh oh!

dschuff commented Sep 18, 2024

Uh oh!

dschuff commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Sep 20, 2024

Uh oh!

sbc100 commented Sep 24, 2024

Uh oh!

Uh oh!

dschuff commented Sep 20, 2024 •

edited

Loading