[COFF] Adjust secrel limit check

[smeenai]

Summary

According to Microsoft's PE/COFF documentation, a SECREL relocation is
"The 32-bit offset of the target from the beginning of its section". By
my reading, the "from the beginning of its section" implies that the
offset is unsigned.

Change from an assertion to an error, since it's possible to trigger
this condition normally for input files with very large sections, and we
should fail gracefully for those instead of asserting.

[smeenai]

Again, I'm not sure how to create a test for this one without creating gigantic (> 2 GiB) intermediate object files. The .bss trick won't work here, since we can't have relocations in a .bss section.

[compnerd]

Sorry I wasn't able to get around to this. Im not sure if we can add a test for this. Generating a 4GiB binary from the assembler takes a while.

[ruiu]

COFF/Chunks.cpp

65 ↗

(On Diff #115779)

Can you use error() instead of assert()? If you can intentionally trigger an assertion, it shouldn't be an assert() but should be handled by error().

[smeenai]

COFF/Chunks.cpp

65 ↗

(On Diff #115779)

I thought about that, but I think an assertion is actually the right thing here. D38005 makes it so that a section larger than 4 GiB will trigger an error, which means relocation processing can assume that as a precondition, and a SecRel larger than 4 GiB implies a section larger than 4 GiB, which implies a precondition violation. Does that seem reasonable?

On the other hand, since D38005 is using error, I suppose it's possible to enter here with a section larger than 4 GiB which you've already issued an error for previously (but processing continued after issuing that error), so that makes it slightly less clear.

[ruiu]

COFF/Chunks.cpp

65 ↗

(On Diff #115779)

When you call error() it won't immediately exit. If you explicitly return if ErrorCount is larger than zero before reaching this function, then yes, it's a precondition and assert would be the best choice.

[smeenai]

Change to error

[smeenai]

Bail out of function in case of error

[ruiu]

LGTM

lld/trunk/COFF/Chunks.cpp

 static void applySecRel(const SectionChunk *Sec, uint8_t *Off,
                         OutputSection *OS, uint64_t S) {
   if (!OS) {
     if (Sec->isCodeView())
       return;
     fatal("SECREL relocation cannot be applied to absolute symbols");
   }
   uint64_t SecRel = S - OS->getRVA();
-  assert(SecRel < INT32_MAX && "overflow in SECREL relocation");
+  if (SecRel > UINT32_MAX) {
+    error("overflow in SECREL relocation in section: " + Sec->getSectionName());
+    return;
+  }
   add32(Off, SecRel);
 }
 
 static void applySecIdx(uint8_t *Off, OutputSection *OS) {
   // If we have no output section, this must be an absolute symbol. Use the
   // sentinel absolute symbol section index.
   uint16_t SecIdx = OS ? OS->SectionIndex : DefinedAbsolute::OutputSectionIndex;
   add16(Off, SecIdx);