This is an archive of the discontinued LLVM Phabricator instance.

[JITLink] Fix splitBlock if there are symbols span across the boundary
ClosedPublic

Authored by steven_wu on Nov 15 2021, 9:38 AM.

Details

Summary

Fix splitBlock so that it can handle the case when the block being
split has symbols span across the split boundary. This is an error
case in general but for EHFrame splitting on macho platforms, there is an
anonymous symbol that marks the entire block. Current implementation
will leave a symbol that is out of bound of the underlying block. Fix
the problem by dropping such symbols when the block is split.

Diff Detail

Event Timeline

steven_wu created this revision.Nov 15 2021, 9:38 AM
steven_wu requested review of this revision.Nov 15 2021, 9:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2021, 9:38 AM
dexonsmith added inline comments.Nov 15 2021, 10:35 AM
llvm/lib/ExecutionEngine/JITLink/JITLink.cpp
217–225

This doesn't seem safe in general, since anonymous symbols aren't necessarily safe to split just because they're anonymous.

I wonder if, rather than return Errors, it'd be better to trust the caller and split blocks anyway (as the code was), but truncate Symbol::size() as necessary to avoid it going past the end of the block it points at.

Two other ideas:

  • Change the LinkGraph parser to set the symbol size to 0 for symbols that are generated just to allow edges to a block.
  • Add a bit to Symbol that says "safe to split", which LinkGraph would set for symbols it generates just to allow edges to a block.

@lhames, WDYT?

steven_wu added inline comments.Nov 15 2021, 10:46 AM
llvm/lib/ExecutionEngine/JITLink/JITLink.cpp
217–225

So the symbol in question is the one created here:

MachOLinkGraphBuilder::addSectionStartSymAndBlock

I don't know how this one is being used but the one marked eh_frame will become redundant after eh_frame splitting. I guess we can shrink the size of section start sym to have size 0 then we can just always split the block and not worry about that. Then any symbols span across the split boundary will be Error.

I wonder if, rather than return Errors, it'd be better to trust the caller and split blocks anyway (as the code was), but truncate Symbol::size() as necessary to avoid it going past the end of the block it points at.

I like this as a short-term solution.

I think this issue exposes limitations in LinkGraph's current design. I'm not sure that we need/want LLVM-style users-lists for Symbols, but if we just added a ref-count to Symbols we could have a .isUnreferenced method that would be really helpful here: Clients could choose to remove unreferenced anonymous symbols (which should always be safe, since they can be recreated easily), and/or truncate named symbols, and then splitBlock could return an Error for any remaining symbols that extend past a split point.

Address review feedback after talking with Lang and Duncan offline.

Update the patch to trust user's action and update symbols to fit in the new block

lhames accepted this revision.Nov 15 2021, 1:49 PM

LGTM. Thanks Steven!

This revision is now accepted and ready to land.Nov 15 2021, 1:49 PM