Skip to content
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

LLD does not preserve symbol types for defsym #46314

Closed
smeenai opened this issue Aug 3, 2020 · 7 comments
Closed

LLD does not preserve symbol types for defsym #46314

smeenai opened this issue Aug 3, 2020 · 7 comments
Assignees
Labels
bugzilla Issues migrated from bugzilla lld:ELF

Comments

@smeenai
Copy link
Collaborator

smeenai commented Aug 3, 2020

Bugzilla Link 46970
Resolution FIXED
Resolved on Nov 16, 2020 13:01
Version unspecified
OS All
Blocks #4440
CC @MaskRay,@nickdesaulniers,@rui314,@smithp35

Extended Description

$ cat f.c
void f() {}

$ clang -c f.c
$ ld.lld -shared --defsym=g=f f.o
$ objdump -T a.out
DYNAMIC SYMBOL TABLE:
00000000000012a0 g DF .text 0000000000000006 f
00000000000012a0 g D .text 0000000000000000 g

f is marked STT_FUNC, but g is not. It might be hard to do in the general case (where e.g. you can have arithmetic in the defsym expression), but in this case, it seems desirable for the alias to get the same symbol type as the original.

From Peter Smith:

There can be a problem on Arm as no interworking will be performed for symbols that are not STT_FUNC.

$ cat h.c
extern void f();
extern void g();
void h() { f(); g(); }

$ clang --target=armv7a-none-eabi -c f.c
$ clang --target=armv7a-none-eabi -c h.c -mthumb
$ ld.lld f.o h.o --defsym g=f # No --shared to prevent a PLT entry.
$ objdump -d a.out
000200e8 :
200e8: b580 push {r7, lr}
200ea: 466f mov r7, sp
200ec: f7ff effa blx 200e4
200f0: f7ff fff8 bl 200e4
200f4: bd80 pop {r7, pc}

The blx to f() is correct as a state change is required. The bl to f() will likely crash the program.

ld.bfd correctly marks g as STT_FUNC so it gets the state change correct for both calls.
00008004 :
8004: b580 push {r7, lr}
8006: 466f mov r7, sp
8008: f7ff effa blx 8000
800c: f7ff eff8 blx 8000
8010: bd80 pop {r7, pc}

@smeenai
Copy link
Collaborator Author

smeenai commented Aug 3, 2020

assigned to @MaskRay

@MaskRay
Copy link
Member

MaskRay commented Aug 9, 2020

This exposes some challenge because our abstraction for symbol assignment (ExprValue and std::function<ExprValue()>) do not track symbol types. We can add an auxiliary data structure or extend ExprValue with an additional symbol type field. We need to make this elegant.

There are also questions about when the symbol type should be retained. I asked on binutils: https://sourceware.org/pipermail/binutils/2020-August/112706.html

For our purposes, supporting just alias = aliasee and not other expressions should probably suffice.

@smeenai
Copy link
Collaborator Author

smeenai commented Aug 12, 2020

Thanks for starting the conversation with binutils! I agree that just supporting alias = aliasee is enough.

Extending ExprValue with either the symbol type or just a pointer to the symbol seem reasonable to me, though I haven't thought about it too deeply. We would also need some way to distinguish between alias = aliasee and more complex expressions, correct? I don't think ExprValue tells us that right now (it just tracks the end value of the expression).

@smeenai
Copy link
Collaborator Author

smeenai commented Aug 19, 2020

This will start blocking an internal codebase soon, so I'm looking into it.

@smeenai
Copy link
Collaborator Author

smeenai commented Aug 20, 2020

Fangrui is fixing this in https://reviews.llvm.org/D86263. Thank you!

@MaskRay
Copy link
Member

MaskRay commented Aug 27, 2020

Fixed by 9670029 (cherry picked to release/11.x)

@nickdesaulniers
Copy link
Member

Just noting that this was a bug affecting the aarch64 Linux kernel's 32b compat VDSO for 4.19.y. (Everything is fixed, just leaving a note).

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla lld:ELF
Projects
None yet
Development

No branches or pull requests

3 participants