This is an archive of the discontinued LLVM Phabricator instance.

[jitlink/rtdydl][checker] Add TargetFlag dependent disassembler switching support
ClosedPublic

Authored by Eymay on Aug 18 2023, 7:19 AM.

Details

Summary

Some targets such as AArch32 make use of TargetFlags to indicate ISA mode. Depending
on the TargetFlag, MCDisassembler and similar target specific objects should be
reinitialized with the correct Target Triple. Backends with similar needs can
easily extend this implementation for their usecase.

The drivers llvm-rtdyld and llvm-jitlink have their SymbolInfo's extended to take
TargetFlag into account. RuntimeDyldChecker can now create necessary TargetInfo
to reinitialize MCDisassembler and MCInstPrinter. The required triple is obtained
from the new getTripleFromTargetFlag function by checking the TargetFlag.

In addition, breaking changes for RuntimeDyld COFF Thumb tests are fixed by making
the backend emit a TargetFlag.

Diff Detail

Event Timeline

Eymay created this revision.Aug 18 2023, 7:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2023, 7:19 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Eymay added a comment.Aug 18 2023, 7:24 AM

This patch needs refactoring in areas such as error handling, backend specialization structure for triple manipulation and state management for triple change.

What if you made RuntimeDyldChecker::getInstPrinter and RuntimeDyld::getDisasssembler take a symbol, then check the target flags in each of those methods so that the callees reduce back to, e.g.

auto Dis = Checker.getDisassembler(Symbol); // TT and TF taken from Checker member variables.

if (auto E = Dis.takeError()) {
  errs() << "Error obtaining disassembler: " << toString(std::move(E))
         << "\n";
  return false; // Return false or take appropriate action.
}
llvm/include/llvm/ExecutionEngine/RuntimeDyldChecker.h
138

What about a getTargetFlags method, and let the client do the bitwise ops?

Eymay edited the summary of this revision. (Show Details)Aug 18 2023, 9:53 AM
Eymay published this revision for review.Aug 18 2023, 9:55 AM
Eymay edited the summary of this revision. (Show Details)
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2023, 9:56 AM
Eymay updated this revision to Diff 551588.Aug 18 2023, 11:47 AM
  • Moved TargetFlag checking into getDisassembler
  • Introduced getTripleFromTargetFlag for backend specialization
  • Replaced hasTargetFlag with getTargetFlag
Eymay marked an inline comment as done.Aug 18 2023, 11:47 AM

What if you made RuntimeDyldChecker::getInstPrinter and RuntimeDyld::getDisasssembler take a symbol, then check the target flags in each of those methods so that the callees reduce back to, e.g.

auto Dis = Checker.getDisassembler(Symbol); // TT and TF taken from Checker member variables.

if (auto E = Dis.takeError()) {
  errs() << "Error obtaining disassembler: " << toString(std::move(E))
         << "\n";
  return false; // Return false or take appropriate action.
}

Thanks for the feedback! It makes sense, moved handling into the getters. Also added getTripleFromTargetFlag function. Now, targets can handle their TargetFlag--Triple mappings inside there.

Eymay edited the summary of this revision. (Show Details)Aug 23 2023, 1:12 AM
Eymay added a subscriber: jobnoorman.

Minor comments aside this is looking great. Once these are fixed I think that this is ready to land.

llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h
625–627

This can be removed now that getTargetFlags has been added.

llvm/include/llvm/ExecutionEngine/RuntimeDyldChecker.h
85–90

This constructor should be removed so that clients are forced to specify a flags value explicitly, even if it's just the default.

llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldChecker.cpp
876–877

This assertion should be removed: 0x1 is only the thumb flag for arm, not other architectures.

Eymay updated this revision to Diff 553212.Aug 24 2023, 11:34 AM
  • removed hasTargetFlags and updated its calls in aarch32 jitlink
  • removed MemoryRegionInfo constructor without the TargetFlag field
  • removed irrelevant assert
Eymay marked 3 inline comments as done.Aug 24 2023, 11:36 AM

Currently RuntimeDyld/ARM/COFF_Thumb.s fails due to not finding TargetFlag set to Thumb Flag in SymInfo. I'm still investigating the issue.

Eymay added inline comments.Aug 24 2023, 11:52 AM
llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldChecker.cpp
931

TODO Added a tri-state for easier debugging, should be removed in the end

944

Somehow this didn't fail but having the same consecutive flag should be failing it. This trickery was done to protect version numbers:

For example armv7-... should not become thumb-.. but instead thumbv7-... I'm a little surprised to not find an API for this case

Eymay added inline comments.Aug 24 2023, 11:57 AM
llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldChecker.cpp
890

Maybe getDisassembler and getInstrPrinter could share code for MRI and MAI?

lhames added inline comments.Aug 24 2023, 12:31 PM
llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldChecker.cpp
890

Looks like MCContext doesn't take ownership -- we're probably leaking them at the moment.

I don't think I'd worry about sharing them (at least not yet -- we could do it as an optimization later). For now I'd be inclined to add a struct to hold on to the MRI and MAI (and any other types that aren't owned) alongside the disassembler / inst-printer.

Eymay updated this revision to Diff 553511.Aug 25 2023, 9:05 AM

Added getJITSymbolFlags to COFF Thumb RuntimeDyld backend to emit Thumb TargetFlag. Fixed a bug in RuntimeDyld/ARM/COFF_Thumb.s.
Added checks to triple switching arm<->thumb into getTripleFromTargetFlag in RuntimDyldChecker.

Eymay added inline comments.Aug 25 2023, 9:13 AM
llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldChecker.cpp
944

checks added for consecutive same triple scenario

Eymay added inline comments.Aug 25 2023, 9:19 AM
llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldChecker.cpp
325

Is there a way we could try alternative triples when there is a fail? The reason is if somehow TargetFlag emission has a problem, the triple is wrong and MCDisassembler fails terribly. How should the error be handled in RuntimeDyld setting?

lhames added inline comments.Aug 29 2023, 10:11 PM
llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldChecker.cpp
325

What would cause TargetFlag emission to be incorrect? Are you worried that we're missing cases in RuntimeDyld?

If we can fix those cases without too much trouble we should. If it's too tricky to fix RuntimeDyld I'd be ok with adding some special-case logic to the checker specifically for arm / thumb: if arm fails try thumb and vice-versa. We should add a fixme to remove this as soon as RuntimeDyld is gone. It might also be good to put the hack behind a mode flag that we set when the checker is created. If we're in "RuntimeDyldMode" then we try the triple swap, if we're in JITLink we don't. This would prevent us from silently masking errors in JITLink.

Eymay added inline comments.Aug 29 2023, 11:46 PM
llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldChecker.cpp
325

I've fixed the issue with RuntimeDyld, MachO emits TargetFlag correctly and I added a fix for COFF. The issue is rather about more informative error messages. I suggest in case of failure the instPrinter can try alternative TargetFlag and give diagnostic about the decoded instruction together with an error message of TargetFlag setting.

Sorry for being late at the party. I was too busy in the last weeks to approach non-trivial reviews. This is looking really good already. Thanks for your help @lhames!!
Here are my 2 cents. Unfortunately, I cannot make inline comments anymore.. might be due to the move from Pharicator to GitHub PRs. I will do my best to keep this readable.


Eymay wrote:
Maybe getDisassembler and getInstrPrinter could share code for MRI and MAI?

lhames wrote:
Looks like MCContext doesn't take ownership -- we're probably leaking them at the moment.
I don't think I'd worry about sharing them (at least not yet -- we could do it as an optimization later). For now I'd be inclined to add a struct to hold on to the MRI and MAI (and any other types that aren't owned) alongside the disassembler / inst-printer.

Agree with @lhames: The code sharing is optional, be we must prevent STI, MRI, MAI and Ctx from getting destroyed once their unique_ptrs go out of scope, because Disassembler only takes raw pointers and no ownership. (Same for getInstPrinter with MRI, MAI, MII and InstPrinter). I see two options:
(1) Define a struct with fields for each of them (plus the Disassembler/InstPrinter itself) and return that. This is what Lang was saying.
(2) Inline the code into the calling functions.

I think (1) is clean but adds more effort. (2) is easier, but it would impact the readability of the evalDecodeOperand function significantly. You could consider to outline the respective error reporting code from evalDecodeOperand (from auto InstPrinter to return std::make_pair([...])) and put the getInstrPrinter code in there. I don't see a problem putting the getDisassembler code directly into decodeInst.


In RuntimeDyldChecker is there a good reason to have these members them as references? Otherwise let's store them as values, pass them to the ctor by value and forward with std::move.

Triple &TT;
SubtargetFeatures &TF;

Eymay wrote:
I suggest in case of failure the instPrinter can try alternative TargetFlag and give diagnostic about the decoded instruction together with an error message of TargetFlag setting.

If that adds significant complexity, then why not keep it as a potential future follow-up? I think the scope of this patch is good as it is now.

Eymay updated this revision to Diff 556136.Sep 7 2023, 6:22 AM

TargetInfo passing is made safer by passing struct. TT and TF are converted to value members rather than references. Minimised boilerplate.

sgraenitz accepted this revision.Sep 7 2023, 6:32 AM

Thanks for the fixes. This looks good to me! I assume all affected tests still pass. So, let's get this landed.

This revision is now accepted and ready to land.Sep 7 2023, 6:32 AM
lhames accepted this revision.Sep 7 2023, 8:42 AM
Eymay updated this revision to Diff 556184.Sep 7 2023, 11:11 AM
Eymay retitled this revision from [jitlink/rtdydl][checker] Construct disassembler at every decodeInst to [jitlink/rtdydl][checker] Add TargetFlag dependent disassembler switching support.
Eymay edited the summary of this revision. (Show Details)

New commit description

This revision was landed with ongoing or failed builds.Sep 8 2023, 12:07 AM
This revision was automatically updated to reflect the committed changes.
TWeaver added a subscriber: TWeaver.Sep 8 2023, 2:18 AM

Hello and good morning from the UK!

It seems as though this patch has caused test failures on the following buildbots:

https://lab.llvm.org/buildbot/#/builders/230/builds/18341
https://lab.llvm.org/buildbot/#/builders/109/builds/73169
https://lab.llvm.org/buildbot/#/builders/67/builds/12597

Is anyone able to a take a look?

Much obliged,
Tom W

My apologies but I'm going to revert this change until the Author can get a good look at the build bot failures.

Eymay reopened this revision.Sep 9 2023, 9:56 AM
This revision is now accepted and ready to land.Sep 9 2023, 9:56 AM
Eymay updated this revision to Diff 556352.Sep 9 2023, 9:56 AM

Fixed buildbot failures by adding missing plumbing for Subtarget Features and CPU string

Eymay added a comment.Sep 9 2023, 10:04 AM

Hi @TWeaver, thanks for reverting. I don't have commit access yet so I appreciate it.

Eymay closed this revision.Sep 11 2023, 2:12 AM

Closed by commit rG9c017a99d570: [jitlink][rtdyld][checker] Re-apply 4b17c81d5a5 with fixes..