Page MenuHomePhabricator

[ELF] Error on archive with missing index
ClosedPublic

Authored by sbc100 on Jun 25 2019, 11:41 AM.

Details

Summary

This matches the wasm lld and GNU ld behavior.

The ELF linker has special handling for bitcode archives but if that
doesn't kick in we probably want to error out rather than silently
ignore the library.

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Jun 25 2019, 11:41 AM
ruiu added a comment.Jun 26 2019, 4:32 AM

I think this is a backward-incompatible change. Previously, you could create an archive containing both bitcode files and ELF files, and this patch disallow that combination, which seems a bit too risky change to me.

This change only effects archives without an index. If you previously created an archive with both bitcode *and* ELF files *and* you didn't create and index, the previous behaviour was to completely ignore the archive. This seems bad to me. The new behaviour is to give an error. We could downgrade it to a warning but both GNU ld and wasm-ld give a hard error in this case.

Are you saying that ignoring these hybrid archives is a behavior that you want to keep?

The previous behaviour was also to ignore archives that contain only ELF files but no index.

ruiu added a comment.Jun 27 2019, 6:42 AM

The previous behaviour was also to ignore archives that contain only ELF files but no index.

Is that true? If an archive has no index, each individual member is handled as if they were surrounded by --start-lib/--end-lib, so they wouldn't be ignored, I think.

I'm pretty certain yes. The old code only loads archives one object at a time if all objects are bitcode. I didn't change that part.

The comment also says exactly that does't it?

Can you take a closer look? I'm pretty sure this change is correct. At the very least an improvement, and not a regression.

MaskRay added inline comments.Jul 1 2019, 8:27 PM
lld/test/ELF/archive-no-index.s
8 ↗(On Diff #206492)

/dev/null if you don't need the output file?

ruiu added a comment.Jul 2 2019, 2:14 AM

LGTM

Yeah, looks like I misunderstood the existing code. Thank you for pointing that out.

sbc100 marked 2 inline comments as done.Jul 2 2019, 7:27 PM
sbc100 added inline comments.
lld/test/ELF/archive-no-index.s
8 ↗(On Diff #206492)

Done. I assume that magically works on windows too somehow.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 2 2019, 7:30 PM
Closed by commit rL364998: [ELF] Error on archive with missing index (authored by sbc, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.