[PATCH] D23161: [ELF][MIPS] Produce a correct and complete set of MIPS ELF header flags

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Writer.cpp:1184
@@ +1183,3 @@
+
+static StringRef getMipsIsaName(uint32_t Flags) {
+  switch (Flags) {
----------------
This and the following two "tostring" functions seem a bit odd to be here in the linker. Don't you think they should actually live in LLVM instead of LLD?

================
Comment at: ELF/Writer.cpp:1242-1262
@@ +1241,23 @@
+       Symtab<ELFT>::X->getObjectFiles()) {
+    uint32_t NewFlags = F->getObj().getHeader()->e_flags;
+    if (NewFlags & EF_MIPS_PIC)
+      // PIC code is inherently CPIC and may not set CPIC flag explicitly.
+      NewFlags |= EF_MIPS_CPIC;
+    if (ResFlags == 0) {
+      ResFlags = NewFlags;
+      continue;
+    }
+    uint32_t NewPic = NewFlags & (EF_MIPS_PIC | EF_MIPS_CPIC);
+    uint32_t ResPic = ResFlags & (EF_MIPS_PIC | EF_MIPS_CPIC);
+
+    // Check PIC / CPIC flags compatibility.
+    if (NewPic && !ResPic)
+      warning("linking non-abicalls code with abicalls file: " + F->getName());
+    if (!NewPic && ResPic)
+      warning("linking abicalls code with non-abicalls file: " + F->getName());
+
+    if (!(NewPic & EF_MIPS_PIC))
+      ResFlags &= ~EF_MIPS_PIC;
+    if (NewPic)
+      ResFlags |= EF_MIPS_CPIC;
+
----------------
atanasyan wrote:
> ruiu wrote:
> > I cannot find a simple rule for this code. What is the expected output for what inputs? Is there a documentation to describe the rule behind it?
> > I cannot find a simple rule for this code. What is the expected output for what inputs? Is there a documentation to describe the rule behind it?
> 
> As usual there is no documentation on this code. The general idea - ELF flags define ISA (R_MIPS_32, R_MIPS_32R2, R_MIPS_64 ...), ABI (R_MIPS_O32, R_MIPS_ABI2 ...) and features flags (R_MIPS_NAN2008, R_MIPS_PIC ...). Some kinds of ISAs are compatible with each other and we need to select the "highest" from input ISAs. For example, if one input file has R_MIPS_32 ISA and another one has R_MIPS_32R2 ISA, we need to put R_MIPS_32R2 into the output file. SOme kinds of ISAs are incompatible. We should reject attempts to link R_MIPS_32R2 and R_MIPS_32R6 ISAs together. The same logic is applicable to ABI and some features flags like R_MIPS_NAN2008 and R_MIPS_FP64. Other features flags like EF_MIPS_NOREORDER, EF_MIPS_MICROMIPS etc can be just merged.
> 
> As to the R_MIPS_PIC and R_MIPS_CPIC flags - PIC code is inherently CPIC and may not set CPIC flag explicitly. But it is a good practice to set it in the output file so we do that. If we merge PIC and non-PIC code, output file cannot be considered as position independent so we clear PIC flag if as soon as we see non-PIC input file. If input file does not have PIC not CPIC flags that means that this file does not follow abicalls convention and we show a warning if linker mix abicalls and non-abicalls files.
Thank you for the explanation. So it seems to be inevitably complicated. Do you think you can split this function into small functions, each of which checks only one thing? I'd imagine we could have getMipsPicFlag(), getMipsArch(), getMipsAbi(), etc.


Repository:
  rL LLVM

https://reviews.llvm.org/D23161