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] symbol not found with PROVIDE script #111478

Closed
DianQK opened this issue Oct 8, 2024 · 10 comments
Closed

[LLD] symbol not found with PROVIDE script #111478

DianQK opened this issue Oct 8, 2024 · 10 comments

Comments

@DianQK
Copy link
Member

DianQK commented Oct 8, 2024

I tried the following code. I expect lld to link successful while also removing unused symbols, but it reports symbol not found: bar.

a.s:

.global _start
_start:
 nop
.section .text.foo,"ax",@progbits
.global foo
foo:
 nop
.section .text.bar,"ax",@progbits
.global bar
bar:
 nop

script.t:

PROVIDE(foo = bar);

commands:

llvm-mc -filetype=obj -triple=x86_64 a.s -o a.o
ld.lld -o a_gc a.o --gc-sections -T script.t

output:

ld.lld: error: script.t:1: symbol not found: bar
ld.lld: error: script.t:1: symbol not found: bar
ld.lld: error: script.t:1: symbol not found: bar

cc @MaskRay ebb326a

@MaskRay
Copy link
Member

MaskRay commented Oct 9, 2024

I cannot reproduce ld.lld: error: script.t:1: symbol not found: bar. Are you testing the latest lld?

foo is not in the symbol table. lld considers it unneeded.

@DianQK
Copy link
Member Author

DianQK commented Oct 9, 2024

I cannot reproduce ld.lld: error: script.t:1: symbol not found: bar. Are you testing the latest lld?

foo is not in the symbol table. lld considers it unneeded.

Sorry, I mistakenly deleted the definition of foo. I may have forgotten to run the llvm-mc command to verify after the deletion. I have now fixed this description.

@MaskRay
Copy link
Member

MaskRay commented Oct 11, 2024

Does #111945 address your use case?

@EugeneZelenko EugeneZelenko added lld:ELF and removed lld labels Oct 11, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 11, 2024

@llvm/issue-subscribers-lld-elf

Author: DianQK (DianQK)

I tried the following code. I expect lld to link successful while also removing unused symbols, but it reports `symbol not found: bar`.

a.s:

.global _start
_start:
 nop
.section .text.foo,"ax",@<!-- -->progbits
.global foo
foo:
 nop
.section .text.bar,"ax",@<!-- -->progbits
.global bar
bar:
 nop

script.t:

PROVIDE(foo = bar);

commands:

llvm-mc -filetype=obj -triple=x86_64 a.s -o a.o
ld.lld -o a_gc a.o --gc-sections -T script.t

output:

ld.lld: error: script.t:1: symbol not found: bar
ld.lld: error: script.t:1: symbol not found: bar
ld.lld: error: script.t:1: symbol not found: bar

cc @MaskRay ebb326a

@tru
Copy link
Collaborator

tru commented Oct 11, 2024

Does this need a backport?

@tru tru moved this from Needs Triage to Needs Pull Request in LLVM Release Status Oct 11, 2024
@DianQK
Copy link
Member Author

DianQK commented Oct 11, 2024

Does #111945 address your use case?

I have verified that #111945 has fixed the build error in rust-lang/rust#131164.

I’d like to ask for your advice on the backport. I reverted the related changes in rust-lang#178. What do you think is better, revert or cherry-pick the new patch?

@DianQK
Copy link
Member Author

DianQK commented Oct 11, 2024

Does this need a backport?

Yes. I think so. :)

MaskRay added a commit that referenced this issue Oct 11, 2024
…to Undefined

Case: `PROVIDE(f1 = bar);` when both `f1` and `bar` are in separate
sections that would be discarded by GC.

Due to `demoteDefined`, `shouldAddProvideSym(f1)` may initially return
false (when Defined) and then return true (been demoted to Undefined).

```
addScriptReferencedSymbolsToSymTable
  shouldAddProvideSym(f1): false
  // the RHS (bar) is not added to `referencedSymbols` and may be GCed
declareSymbols
  shouldAddProvideSym(f1): false
markLive
demoteSymbolsAndComputeIsPreemptible
  // demoted f1 to Undefined
processSymbolAssignments
  addSymbol
    shouldAddProvideSym(f1): true
```

The inconsistency can cause `cmd->expression()` in `addSymbol` to be
evaluated, leading to `symbol not found: bar` errors (since `bar` in the
RHS is not in `referencedSymbols` and is GCed) (#111478).

Fix this by adding a `sym->isUsedInRegularObj` condition, making
`shouldAddProvideSym(f1)` values consistent. In addition, we need a
`sym->exportDynamic` condition to keep provide-shared.s working.

Fixes: ebb326a

Pull Request: #111945
@DianQK
Copy link
Member Author

DianQK commented Oct 12, 2024

/cherry-pick 1c6688a

@DianQK DianQK closed this as completed Oct 12, 2024
@github-project-automation github-project-automation bot moved this from Needs Pull Request to Done in LLVM Release Status Oct 12, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 12, 2024

Failed to cherry-pick: 1c6688a

https://github.com/llvm/llvm-project/actions/runs/11301798093

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

@DianQK DianQK reopened this Oct 12, 2024
@github-project-automation github-project-automation bot moved this from Done to Needs Triage in LLVM Release Status Oct 12, 2024
DianQK pushed a commit to DianQK/llvm-project that referenced this issue Oct 13, 2024
…to Undefined

Case: `PROVIDE(f1 = bar);` when both `f1` and `bar` are in separate
sections that would be discarded by GC.

Due to `demoteDefined`, `shouldAddProvideSym(f1)` may initially return
false (when Defined) and then return true (been demoted to Undefined).

```
addScriptReferencedSymbolsToSymTable
  shouldAddProvideSym(f1): false
  // the RHS (bar) is not added to `referencedSymbols` and may be GCed
declareSymbols
  shouldAddProvideSym(f1): false
markLive
demoteSymbolsAndComputeIsPreemptible
  // demoted f1 to Undefined
processSymbolAssignments
  addSymbol
    shouldAddProvideSym(f1): true
```

The inconsistency can cause `cmd->expression()` in `addSymbol` to be
evaluated, leading to `symbol not found: bar` errors (since `bar` in the
RHS is not in `referencedSymbols` and is GCed) (llvm#111478).

Fix this by adding a `sym->isUsedInRegularObj` condition, making
`shouldAddProvideSym(f1)` values consistent. In addition, we need a
`sym->exportDynamic` condition to keep provide-shared.s working.

Fixes: ebb326a

Pull Request: llvm#111945

(cherry picked from commit 1c6688a)
@DianQK
Copy link
Member Author

DianQK commented Oct 13, 2024

Backport PR: #112136

@DianQK DianQK closed this as completed Oct 13, 2024
@github-project-automation github-project-automation bot moved this from Needs Triage to Done in LLVM Release Status Oct 13, 2024
MaskRay added a commit that referenced this issue Oct 15, 2024
The inaccurate #111945 condition fixes a PROVIDE regression (#111478)
but introduces another regression: in a DSO link, if a symbol referenced
only by bitcode files is defined as PROVIDE_HIDDEN, lld would not set
the visibility correctly, leading to an assertion failure in
DynamicReloc::getSymIndex (https://reviews.llvm.org/D123985).
This is because `(sym->isUsedInRegularObj || sym->exportDynamic)` is
initially false (bitcode undef does not set `isUsedInRegularObj`) then
true (in `addSymbol`, after LTO compilation).

Fix this by making the condition accurate: use a map to track defined
symbols.

Reviewers: smithp35

Reviewed By: smithp35

Pull Request: #112386
DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this issue Oct 16, 2024
…to Undefined

Case: `PROVIDE(f1 = bar);` when both `f1` and `bar` are in separate
sections that would be discarded by GC.

Due to `demoteDefined`, `shouldAddProvideSym(f1)` may initially return
false (when Defined) and then return true (been demoted to Undefined).

```
addScriptReferencedSymbolsToSymTable
  shouldAddProvideSym(f1): false
  // the RHS (bar) is not added to `referencedSymbols` and may be GCed
declareSymbols
  shouldAddProvideSym(f1): false
markLive
demoteSymbolsAndComputeIsPreemptible
  // demoted f1 to Undefined
processSymbolAssignments
  addSymbol
    shouldAddProvideSym(f1): true
```

The inconsistency can cause `cmd->expression()` in `addSymbol` to be
evaluated, leading to `symbol not found: bar` errors (since `bar` in the
RHS is not in `referencedSymbols` and is GCed) (llvm#111478).

Fix this by adding a `sym->isUsedInRegularObj` condition, making
`shouldAddProvideSym(f1)` values consistent. In addition, we need a
`sym->exportDynamic` condition to keep provide-shared.s working.

Fixes: ebb326a

Pull Request: llvm#111945
DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this issue Oct 16, 2024
The inaccurate llvm#111945 condition fixes a PROVIDE regression (llvm#111478)
but introduces another regression: in a DSO link, if a symbol referenced
only by bitcode files is defined as PROVIDE_HIDDEN, lld would not set
the visibility correctly, leading to an assertion failure in
DynamicReloc::getSymIndex (https://reviews.llvm.org/D123985).
This is because `(sym->isUsedInRegularObj || sym->exportDynamic)` is
initially false (bitcode undef does not set `isUsedInRegularObj`) then
true (in `addSymbol`, after LTO compilation).

Fix this by making the condition accurate: use a map to track defined
symbols.

Reviewers: smithp35

Reviewed By: smithp35

Pull Request: llvm#112386
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

5 participants