[PATCH] D62464: [PPC32] Improve 32-bit PowerPC

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/SyntheticSections.cpp:2313
+    // Then write PLTresolve.
+    Target->writePltHeader(Buf + 4 * N);
+    return;
----------------
MaskRay wrote:
> ruiu wrote:
> > MaskRay wrote:
> > > ruiu wrote:
> > > > Hmm, so we don't call writePlt() if a target is PPC32?
> > > On PPC32, how PLT works is different from targets (it is still similar to PPC64). I've added more comments in PPC::writePltHeader.
> > So, this is not really a PLT header but a PPC32-specific mechanism. Is writePltHeader a correct name? Maybe we should rename this writePPC32PltResolver or something like that?
> `.glink` is indeed a PPC-specific mechanism (PPC64 moves the "footer" (PLTresolve) to the "header" so it looks more conventional).
> 
> The `PPC` class is in an unnamed namespace so I cannot access it from SyntheticSections.cpp.  If you still think `writePPC32PltResolver` is the way to go, I can extract it from the class and place it into the `lld::elf::` namespace. I still feel it may not worth the trouble.
> 
> ```
> namespace {
> class PPC final : public TargetInfo {
> public:
> ...
>   void writePltHeader(uint8_t *Buf) const override;
> ```
I feel like a PPC32-specific knowledge such as the actual machine instruction code leaks here, which is I think not ideal. How about moving the code to write `b PLTresolve` to writePltHeader and rename it writePPC32PltSection? Currently, a concrete implementation to write the first half of PPC32's PLT section is in this function while the other is in writePltHeader, which is not actually a header but a trailer. Nothing is really complicated, but there are perhaps too many things that are slightly confusing.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62464/new/

https://reviews.llvm.org/D62464