MCMacho: Allow __thread_ptr section after dwarf sections

[MatzeB]

Summary

r232842 added an assert that fires with the included testcase. Add an exception for '__thread_ptr' to canGoAfterDWARF().

Note: I don't know much about MC or whether this is the right thing to do. At a first glance this code looks like we should rather have a blacklist of sections that must not go after the dwarf sections rather than having a whitelist of sections that are allowed to go. (For example if you read ARMAsmPrinter::EmitEndOfAsmFile() there is another section called "Drectve" that is output after the debug info...

[aprantl]

That means the location of the __thread_ptr section can't affect relaxations in the code or have any similar such effects?
If that is the case, this LGTM.

test/MC/MachO/ARM/no-tls-assert.ll

2 ↗

(On Diff #86382)

It would be better to check the ASM output for the section switch commands. Otherwise this test succeeds even if llc is symlinked to /bin/true ;-)

[grosbach]

Nothing that can have relocations or fixups to or from it should go after the debug info. This patch doesn't seem right to me.

[MatzeB]

test/MC/MachO/ARM/no-tls-assert.ll

2 ↗

(On Diff #86382)

The assert would not trigger when writing assembly, but I can probably pipe the output through llvm-objdump.

[grosbach]

test/MC/MachO/ARM/no-tls-assert.ll

2 ↗

(On Diff #86382)

That's concerning. What is the assembler doing differently? Any output from the object emitter directly from the compiler should be reproducible when going through assembly.

[MatzeB]

test/MC/MachO/ARM/no-tls-assert.ll

2 ↗

(On Diff #86382)

The assert is only in MCMachoStreamer. I assume when writing an assembly file the order in which we see the .section directives doesn't influence the order of the sections in the object file? That would explain why we wouldn't need to enforce the order for assembly.

[MatzeB]

In D29315#661848, @grosbach wrote:

Nothing that can have relocations or fixups to or from it should go after the debug info. This patch doesn't seem right to me.

What's the motivation for this rule? Less work when striping a binary?

Also we do allow the __DATA/__nl_symbol_ptr section after the dwarf sections today it seems, shouldn't __DATA/__thread_ptr behave very similar?

[grosbach]

In D29315#662038, @MatzeB wrote:

In D29315#661848, @grosbach wrote:

Nothing that can have relocations or fixups to or from it should go after the debug info. This patch doesn't seem right to me.

What's the motivation for this rule? Less work when striping a binary?

Also we do allow the __DATA/__nl_symbol_ptr section after the dwarf sections today it seems, shouldn't __DATA/__thread_ptr behave very similar?

The main thing is that debug info can get so large that putting it between things results in relocations being out of range of what the file format can represent. So we put all the stuff than can reference each other w/ relocations up front next to one another. TEXT relocations in particular due to the references being start-of-file relative offsets. It might be OK(ish) for DATA? That would line up with various commit messages. That we're tripping the assert() might imply it's not? Worth exploring a bit to be sure. Debugging when this stuff goes wrong is unpleasant.

For some history on that part, see r231898 and r232842.

test/MC/MachO/ARM/no-tls-assert.ll

2 ↗

(On Diff #86382)

Barring something explicitly overriding the behaviour, the order of the .section directives is exactly what determines the order in the object file. That's why a long time ago in a toolchain far, far away we put a preamble in the .s files w/ a bunch of .section directives. That changed with r232842.

Even with that, however, the section ordering should be coming out the same from a .s file. That's a different quirk and needs understood.

[MatzeB]

In D29315#662118, @grosbach wrote:

In D29315#662038, @MatzeB wrote:

In D29315#661848, @grosbach wrote:

Nothing that can have relocations or fixups to or from it should go after the debug info. This patch doesn't seem right to me.

What's the motivation for this rule? Less work when striping a binary?

Also we do allow the __DATA/__nl_symbol_ptr section after the dwarf sections today it seems, shouldn't __DATA/__thread_ptr behave very similar?

The main thing is that debug info can get so large that putting it between things results in relocations being out of range of what the file format can represent. So we put all the stuff than can reference each other w/ relocations up front next to one another. TEXT relocations in particular due to the references being start-of-file relative offsets. It might be OK(ish) for DATA? That would line up with various commit messages. That we're tripping the assert() might imply it's not? Worth exploring a bit to be sure. Debugging when this stuff goes wrong is unpleasant.

For some history on that part, see r231898 and r232842.

My impression is that the ordering decisions here are mostly driven by rdar://15623193 which is motivates them with binary diffing / reproducibility of the text/data section independently of dwarf being emitted or omitted. With that motivation we should indeed output __DATA/__nl_symbol_ptr and __DATA/__thread_ptr before the dwarf sections.

Note however that we never did this so far and r232842 specifically adds an exception for __nl_symbol_ptr, so in the short term (=clang-4.0 release) I'd like to go ahead with just adding a similar exception for __thread_ptr as proposed here.

It is probably easy to fix the ordering for both nl_symbol_ptr and thread_ptr but I'd like to attack that in a separate patch for llvm ToT only.

[grosbach]

Aha! Yes, that was the piece I'd forgotten. Thank you.

I agree that keeping changes minimal for 4.0 and doing the more interesting long term things on trunk is best.

llvm/trunk/lib/MC/MCMachOStreamer.cpp

if (SegName == "__IMPORT") {
 
     if (SecName == "__pointers")
       return true;
   }
 
   if (SegName == "__TEXT" && SecName == "__eh_frame")
     return true;
 
-  if (SegName == "__DATA" && SecName == "__nl_symbol_ptr")
+  if (SegName == "__DATA" && (SecName == "__nl_symbol_ptr" ||
+                              SecName == "__thread_ptr"))
     return true;
 
   return false;
 }
 
 void MCMachOStreamer::ChangeSection(MCSection *Section,
                                     const MCExpr *Subsection) {
   // Change the section normally.

llvm/trunk/test/MC/MachO/ARM/no-tls-assert.ll

+; RUN: llc -filetype=obj -o - %s | llvm-objdump -section-headers - | FileCheck %s
+; This should not trigger the "Creating regular section after DWARF" assert.
+; CHECK: __text
+; CHECK: __thread_ptr  00000004
+target triple = "thumbv7-apple-ios9.0.0"
+
+@b = external thread_local global i32
+define i32* @func(i32 %a) !dbg !9 {
+  ret i32* @b
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6, !7}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug)
+!1 = !DIFile(filename: "r.ii", directory: "/")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{i32 1, !"min_enum_size", i32 4}
+!7 = !{i32 1, !"PIC Level", i32 2}
+!9 = distinct !DISubprogram(name: "func", scope: !1, file: !1, line: 4, type: !10, isLocal: false, isDefinition: true, scopeLine: 4, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!10 = !DISubroutineType(types: !11)
+!11 = !{null, !12}
+!12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!13 = !DILocalVariable(name: "a", arg: 1, scope: !9, file: !1, line: 4, type: !12)
+!14 = !DIExpression()