[PATCH] D45642: [PPC64] V2 abi: Add glink section for lazy symbol resolution.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/SyntheticSections.cpp:1206
+  // for the V2 abi. I'm using an endianess check for now, but this isn't truly
+  // appropriate becuase there is no reason you couldn't use the V2 abi on a BE
+  // machine.
----------------
bec**au**se


================
Comment at: ELF/SyntheticSections.cpp:1208
+  // machine.
+  if (Config->EMachine == EM_PPC64 && ELF64LEKind &&
+      !InX::Glink->empty()) {
----------------
Did you mean `Config->EMachine == EM_PPC64 && Config->IsLE` ?

Given the comment, should `Config->IsLE` check be omitted?

Actually, it seems it could be the following then.

`if (InX::Glink && !InX::Glink->empty()) { ...`


================
Comment at: ELF/SyntheticSections.cpp:2711
+  // There is one resolver stub for each GotPlt entry.
+  assert(InX::GotPlt);
+  return InX::GotPlt->getEntryCount();
----------------
This assert does not make much sense actually. With or without it we are going to crash
anyways.


================
Comment at: ELF/SyntheticSections.cpp:2721
+  if (empty())
+    return;
+
----------------
Seems to be excessive check for me. I would expect we remove the synthetic section from
the output if it is an empty one. Hence I think we should never reach `writeTo` for an empty section.


================
Comment at: ELF/SyntheticSections.cpp:2745
+  const uint32_t BranchOp = 0x48000000;
+  const uint32_t BranchOffsetMask =  0x03FFFFFc;
+  // The entry written to 'Offset' must branch to the start of
----------------
You use these only in a single place, so I would suggest to inline them.


================
Comment at: ELF/Writer.cpp:400
+
+
+  if (Config->EMachine == EM_PPC64) {
----------------
Please remove the excessive empty line.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D45642