Segfault in AArch64 backend with -g and -mbig-endian

[olista01]

Summary

Fix a null pointer dereference when trying to swap the endianness of fixups in the .eh_frame section in the AArch64 backend.

[echristo]

lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

538

Fixup to a null section? How's that happen?

[olista01]

lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

538

This is the section associated with the value to put into the fixup location, not the section containing the fixup location. This can be null if:

• It is a symbol which is not defined by this translation unit
• It is the difference between two temporary symbols, for example to find the length of a debug data structure.

[olista01]

Ping?

[rengolin]

lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

538

Is this a null section or not an ELF section? Your check seems to imply the latter, while I believe it's the former.

In that case, this would be better fixed in the caller, since the result of a null section means you're applying a fixup to a null section and propagating the error.

[olista01]

lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

538

MCExpr::FindAssociatedSection can return nullptr when the expression is a reference to a symbol not defined in this translation unit, so we do have to be able to handle nullptr here without it being an error.

[olista01]

Ping?

[rengolin]

lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

538

Right, so, to reiterate Eric's idea:

A fixup having a location that:

1. was not defined in this unit doesn't make much sense
2. is the result of removed temporary value, should have been removed when the values got out of context

If the problem here is 1, than we need to know why we're trying to reach non-existent sections in the first place. If the problem is 2, we need to know how to delete the fixups related to just deleted values, so we don't come here with null pointers.

But, that's not necessarily part of this patch, since FindAssociatedSection() does return nullptr on those cases.

I'll defer to Eric to approve this.

[olista01]

Eric: ping?

[olista01]

Ping?

[rengolin]

Oliver,

I think there are two things, and your patch fixes one that needs fixing, while there is another that might not need fixing.

The patch looks good to me, feel free to commit. The ELF section needs further analysis, but that's nothing to do with this patch.

Sorry for the delay.

cheers,
--renato

[echristo]

I'm not sure we should paper over this possible problem.

I haven't had a chance to look into the comment here:

"A fixup having a location that:

was not defined in this unit doesn't make much sense
is the result of removed temporary value, should have been removed when the values got out of context"

and Oliver hasn't replied yet either. I do think it's relevant to this patch.

lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

538

Ah right. Thanks.

[rengolin]

So, in this case, the revert need only to happen if the section is .eh_frame. If the section is empty, it's not .eh_frame and the local logic is not wrong.

That's not saying there isn't a problem and that it doesn't need fixing, it just says that this is not the point of *this* patch. What'd be the problem of fixing this and creating a new bug for the empty section? What guarantees that this is the only case where the section is empty? I believe that the right thing to do is to fill the section code with asserts, so we can pinpoint the origin of the problem. But none of that would make this fix wrong in the future.

Furthermore, the function does return null when it doesn't find the section, so if that was really wrong it should assert in the first place. I don't understand what the problem is in this case, can you elaborate?

[olista01]

I have committed this as revision 218311, and I will find some time later this week to check for a deeper problem.

[rengolin]

Thanks Oliver.

I do agree with Eric that we must look into this problem ASAP. Have you created a bug for it? Please copy both of us on it, so we're aware of your progress.

cheers,
--renato

lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

 }
 
 void ELFAArch64AsmBackend::applyFixup(const MCFixup &Fixup, char *Data,
                                       unsigned DataSize, uint64_t Value,
                                       bool IsPCRel) const {
   // store fixups in .eh_frame section in big endian order
   if (!IsLittleEndian && Fixup.getKind() == FK_Data_4) {
     const MCSection *Sec = Fixup.getValue()->FindAssociatedSection();
-    const MCSectionELF *SecELF = static_cast<const MCSectionELF *>(Sec);
-    if (SecELF->getSectionName() == ".eh_frame")
+    const MCSectionELF *SecELF = dyn_cast_or_null<const MCSectionELF>(Sec);
+    if (SecELF && SecELF->getSectionName() == ".eh_frame")
       Value = ByteSwap_32(unsigned(Value));
   }
   AArch64AsmBackend::applyFixup (Fixup, Data, DataSize, Value, IsPCRel);
 }
 }
 
 MCAsmBackend *llvm::createAArch64leAsmBackend(const Target &T,
                                             const MCRegisterInfo &MRI,

test/DebugInfo/AArch64/big-endian.ll

+; RUN: llc %s -filetype=asm -o -
+
+target datalayout = "E-m:e-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64_be--none-eabi"
+
+@a = common global i32 0, align 4
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!8, !9}
+!llvm.ident = !{!10}
+
+!0 = metadata !{i32 786449, metadata !1, i32 12, metadata !"clang version 3.6.0 ", i1 true, metadata !"", i32 0, metadata !2, metadata !2, metadata !2, metadata !3, metadata !2, metadata !"", i32 1} ; [ DW_TAG_compile_unit ] [/work/validation/-] [DW_LANG_C99]
+!1 = metadata !{metadata !"-", metadata !"/work/validation"}
+!2 = metadata !{}
+!3 = metadata !{metadata !4}
+!4 = metadata !{i32 786484, i32 0, null, metadata !"a", metadata !"a", metadata !"", metadata !5, i32 1, metadata !7, i32 0, i32 1, i32* @a, null} ; [ DW_TAG_variable ] [a] [line 1] [def]
+!5 = metadata !{i32 786473, metadata !6}          ; [ DW_TAG_file_type ] [/work/validation/<stdin>]
+!6 = metadata !{metadata !"<stdin>", metadata !"/work/validation"}
+!7 = metadata !{i32 786468, null, null, metadata !"int", i32 0, i64 32, i64 32, i64 0, i32 0, i32 5} ; [ DW_TAG_base_type ] [int] [line 0, size 32, align 32, offset 0, enc DW_ATE_signed]
+!8 = metadata !{i32 2, metadata !"Dwarf Version", i32 4}
+!9 = metadata !{i32 2, metadata !"Debug Info Version", i32 1}
+!10 = metadata !{metadata !"clang version 3.6.0 "}