Details
Diff Detail
- Repository
- rLLD LLVM Linker
- Build Status
Buildable 18475 Build 18475: arc lint + arc unit
Event Timeline
wasm/Driver.cpp | ||
---|---|---|
305 | 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 | ||
45 | Move this to Common/Strings.cpp. | |
53 | I'd think you should move this function to Common/ErrorHandler.cpp. | |
58 | Move this to Common/ErrorHandler.cpp. |
wasm/Driver.cpp | ||
---|---|---|
280 | 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 | ||
---|---|---|
58 | Can't you just declare the class without including that header? |
wasm/Driver.cpp | ||
---|---|---|
279 | 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. | |
291 | 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 | ||
280 | 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 | ||
280 | 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 | ||
---|---|---|
280 | 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.