Details
Diff Detail
- Repository
- rLLD LLVM Linker
- Build Status
- Buildable 18426 - Build 18426: arc lint + arc unit 
Event Timeline
| wasm/Driver.cpp | ||
|---|---|---|
| 307 | ELF lld supports --thinlto-jobs=, --lto-partitions=, etc as well as --plugin-opt=jobs=, --plugin-opt=partitions=, etc for compatibility with the LTO plugin for GNU gold. Since you are creating a new linker from scratch now, I don't think that makes sense to keep --plugin-opt options. I'd use only non-"--plugin-opt=" options. | |
| wasm/InputFiles.cpp | ||
| 378 | No else after return | |
| wasm/InputFiles.h | ||
| 73 | nit: add a blank line before a comment. | |
| wasm/InputFiles.cpp | ||
|---|---|---|
| 374 | What does Ref means? I'd name this just Name. | |
| 392 | fatal is to report corrupted files. Since this case is to report a miseuse of the command, you should use error. | |
| wasm/LTO.cpp | ||
| 44 | Move this to Common/Strings.cpp. | |
| 52 | I'd think you should move this function to Common/ErrorHandler.cpp. | |
| 57 | Move this to Common/ErrorHandler.cpp. | |
| wasm/Driver.cpp | ||
|---|---|---|
| 281 | Should we implement -u in the same way as in the ELF linker? With this implementation it doesn't look like it will work correctly for undefined data and globals. | |
| wasm/InputFiles.cpp | ||
| 383 | I think you also want to copy the symbol's binding here and below. | |
| wasm/SymbolTable.cpp | ||
| 200 | This function seems to be unused. | |
| wasm/LTO.cpp | ||
|---|---|---|
| 57 | Can't you just declare the class without including that header? | |
| wasm/Driver.cpp | ||
|---|---|---|
| 280 | It feels like handle prefix implies that it's a side-effect-only function, though this function returns a new object. Perhaps addUndefined is a better name. | |
| 292 | We do this not in Driver::main() but in elf::link() in the ELF linker. | |
| wasm/InputFiles.cpp | ||
| 392 | Please fix. | |
| wasm/LTO.h | ||
| 52 | Buff -> Buf for consistency with ELF. | |
| wasm/SymbolTable.cpp | ||
| 201 | Why is this function empty? | |
- feedback and rebase onto separate change
| wasm/InputFiles.cpp | ||
|---|---|---|
| 392 | Both COFF and ELF use fatal() when when passed bitcode of the wrong arch, but I guess that just means that they need fixing. | |
| test/wasm/lto/lto-start.ll | ||
|---|---|---|
| 3 | More tests please. I would expect to see a ThinLTO test as well as tests for the new flags. | |
| wasm/Config.h | ||
| 32–50 | Out of these new options it looks like you are only initializing --lto-partitions and --save-temps during option parsing. You probably want to either initialize the remaining ones or remove them. | |
| wasm/Driver.cpp | ||
| 281 | Please address. | |
| wasm/InputFiles.cpp | ||
| 383 | Please address. | |
| test/wasm/lto/lto-start.ll | ||
|---|---|---|
| 3 | I agree. I was thinking of landing this initial version and iterating (see the change description), but perhaps its best not to land in a half baked state. I'm happy to add more tests and make this more complete. | |
| wasm/Driver.cpp | ||
| 281 | You are correct. Right now with wasm-ld -u flag only works for function symbols. This is a current limitation of the wasm linker, not something that this change introduces. I agree we should address that, but that would be a separate change I think. | |
| wasm/Driver.cpp | ||
|---|---|---|
| 281 | Yes it should be a separate change, just something I noticed while reviewing this code. | |
I think this needs a couple more tests:
- Test that the binding is copied correctly to the LLD symbol. You can probably do this with a test that shows that strong/weak resolution produces the correct symbol.
- Test that VisibleToRegularObj is set correctly when building an executable. See test/ELF/lto/internalize-basic.ll for an example.
| ELF/Options.td | ||
|---|---|---|
| 200 ↗ | (On Diff #148982) | You probably want to undo this part or do it separately. | 
| test/wasm/lto/thinlto.ll | ||
| 18 ↗ | (On Diff #148982) | You should be able to do the same test here as in the other cases. I've updated this test in the ELF linker to do the same in r333480. | 
More tests please. I would expect to see a ThinLTO test as well as tests for the new flags.