Page MenuHomePhabricator

[COFF] Add address-taken import thunks to the fid table
ClosedPublic

Authored by rnk on Feb 27 2019, 2:25 PM.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

rnk created this revision.Feb 27 2019, 2:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2019, 2:25 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript

So after some further digging I was wrong, and my particular bug isn't to do with S->kind(). :(

The function that I'm interested in (a Rust function, on arm64, if it matters) is apparently failing the IMAGE_SYM_DTYPE_FUNCTION test. Is there any other category of thing that it could be?

The function that I'm interested in (a Rust function, on arm64, if it matters) is apparently failing the IMAGE_SYM_DTYPE_FUNCTION test. Is there any other category of thing that it could be?

Its getType() is 0, if that means anything.

rnk marked an inline comment as done.Feb 27 2019, 3:19 PM
rnk added inline comments.
lld/test/COFF/guardcf-thunk.s
28 ↗(On Diff #188625)

I think IMAGE_SYM_DTYPE_FUNCTION is set by the .type directive in this .def/.endef block. I guess what we're doing doesn't match MSVC, but it also seems reasonable. Is the Rust function written in assembly, or is it emitted in LLVM? We should be setting the appropriate type...

dmajor added inline comments.Feb 27 2019, 3:25 PM
lld/test/COFF/guardcf-thunk.s
28 ↗(On Diff #188625)

The function is on_tls_callback at https://github.com/rust-lang/rust/blob/master/src/libstd/sys/windows/thread_local.rs#L201.

(It's unclear to me if the linker magic related to TLS callbacks is related to this issue. This is just the first function we fail at. I don't know if there are more "normal" functions that we'd fail on after it.)

rnk marked an inline comment as done.Feb 27 2019, 4:29 PM
rnk added inline comments.
lld/test/COFF/guardcf-thunk.s
28 ↗(On Diff #188625)

Well, most global variables also seem to have type "notype", so the only way we could make this work is by dropping the symbol type check completely and just looking at the section executability.

rnk marked an inline comment as done.Feb 27 2019, 4:36 PM
rnk added inline comments.
lld/test/COFF/guardcf-thunk.s
28 ↗(On Diff #188625)

I checked, and MSVC performs this symbol type check, so we are being compatible, and I don't think we should remove it.

dmajor accepted this revision.Feb 27 2019, 6:17 PM

Oh you know what, I bet it's https://reviews.llvm.org/D53540#1326473. Rust has an LLVM roll in progress as I write this, so this should just sort itself out.

Sorry for making you write a "needless" patch, though I'm glad that at least the original bug you filed is fixed!

This revision is now accepted and ready to land.Feb 27 2019, 6:17 PM
This revision was automatically updated to reflect the committed changes.