[XCOFF] Don't emit non-external labels in the symbol table and handle MCSA_LGlobal

[daltenty]

Summary

We need to handle the MCSA_LGlobal case in emitSymbolAttribute for functions marked internal in the IR so that the
appropriate storage class is emitted on the function descriptor csect. As part of this we need to make sure that external
labels are not emitted into the symbol table, so we don't emit the descriptor label in the object writing path.

[daltenty]

• Add missing newline

[jasonliu]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

688 ↗

(On Diff #245874)

I understand this is to make the object writing case and assembly case to be equivalent.
But I think this changes the original design intent. The intent was when it is a static function, we do not need an label in the symbol table at all. (@Xiangling_L Correct me if I'm wrong.) Also I'm not sure if it actually make sense to have a label in the symbol table that is not external.
So in order to not change the design intent, I think the way to fix is the other way around. i.e although we registered a symbol, but if it is a label and is C_HIDEXT, do not add the symbol into csect's symbol list. So that we do not need to emit the internal label in the object file generation.

llvm/lib/MC/MCXCOFFStreamer.cpp

17

Do we need this include at all?

llvm/test/CodeGen/PowerPC/aix-internal.ll

9

Remove #0

21

Without the present of SymbolType, I'm not sure what are we testing here. Are we expecting a label or a csect, or both?

[daltenty]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

688 ↗

(On Diff #245874)

Hmm, I do agree this does diverge from the original design intent, though I'm not sure that is avoidable for reasons outlined below.

With regard to whether it actually makes sense to have a label in the symbol table that is not external, this seems to be the entire intent of the the .lgobl directive. The directive seems to be for statics that need to be kept in the symbol table (I'm assuming for relocation and other purposes). For that reason, I don't think it makes sense to eliminate them in the XCOFFObjectWriter as proposed.

Given that we can't just eliminate them in the XCOFFObjectWriter. I think we have two options:

1. Change the fact that we don't emit the lgobl on the label in the assembly path. Then we will be consistent between the object writing and assembly writing paths. Though it does diverge from the original design and what both xl and gcc do, the extra label in the symbol table should be fairly harmless.

2. Perhaps it would be less confusing to eliminate this particular label altogether? We ran into difficulties with it with the TOC as well. We already have the csect symbol which we can use in the place of this symbol in most places. This might need some larger refactoring for places we try to get a function's symbol though.

[jasonliu]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

688 ↗

(On Diff #245874)

The two options listed are more like a workaround to the problem.
I think the real issue underneath is that, as an
integrated-assembler, we should be able to handle case like

.csect a[xx]
b:
...

Label b does not appear in the symbol table.

.globl/.lglobl/.weak/.extern b
.csect a[xx]
b:
...

b should appear in the symbol table.

But right now, I don't think we could handle that. And we might want to fix this instead.

[Xiangling_L]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

688 ↗

(On Diff #245874)

Yes, your understanding is correct. Omitting function descriptor label symbol when it's a static function is an optimization.

[daltenty]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

688 ↗

(On Diff #245874)

I agree this is the underlying issue. Ideally, we should be able to handle this with MCSymbol which are Temporary. That seems to be how other targets handle user written assembler temporaries.

In this case the label here would be marked as Temporary and we would not emit it naturally in the symbol table. This would require some major restructuring of how this symbol gets created however, since this property can only be set at creation time currently.

However, we have another bit of an issue with this approach. It looks like other assembly dialects use a prefix to indicate this, and set the attribute when the symbol is created, where we use a directive so we cannot know this at the time the symbol is created. So we'd need a mechanism to set it afterwards anyway.

[daltenty]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

688 ↗

(On Diff #245874)

Summarizing offline discussion: We will use the external attribute to determine if the label should be emitted into the symbol table. In a follow on patch we will have to set that appropriately for the assembly parsing case when encountering .lgobl and family directives.

[daltenty]

• Use external to determine if we should emit the symbol table entry
• Revert to not emitting linkage on the descriptor if it's a static
• Update test to refer to csect

[daltenty]

llvm/lib/MC/MCXCOFFStreamer.cpp

17

This should probably be here to include what we use.

[jasonliu]

Some nit comments. Otherwise, LGTM.

llvm/lib/MC/MCXCOFFStreamer.cpp

13

Why do we move this "#include" here?

llvm/lib/MC/XCOFFObjectWriter.cpp

351

nit: Maybe avoid double negate?
Suggestion:
Only put label into the symbol table when it is an external label.

[daltenty]

• Update tests with labels that shouldn't be in the objectfile

[daltenty]

llvm/lib/MC/MCXCOFFStreamer.cpp

13

Per the style guide, a "Main Module Header" should come first in the include list. This is where clang-format will place it.

[daltenty]

• Update comment

[daltenty]

Rebase

llvm/lib/MC/MCXCOFFStreamer.cpp

 //===- lib/MC/MCXCOFFStreamer.cpp - XCOFF Object Output -------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file assembles .s files and emits XCOFF .o object files.
 //
 //===----------------------------------------------------------------------===//
 
+#include "llvm/MC/MCXCOFFStreamer.h"
 #include "llvm/BinaryFormat/XCOFF.h"
 #include "llvm/MC/MCAsmBackend.h"
 #include "llvm/MC/MCCodeEmitter.h"
+#include "llvm/MC/MCDirectives.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCSymbolXCOFF.h"
-#include "llvm/MC/MCXCOFFStreamer.h"
 #include "llvm/Support/TargetRegistry.h"
 
 using namespace llvm;
 
 MCXCOFFStreamer::MCXCOFFStreamer(MCContext &Context,
                                  std::unique_ptr<MCAsmBackend> MAB,
                                  std::unique_ptr<MCObjectWriter> OW,
                                  std::unique_ptr<MCCodeEmitter> Emitter)
     : MCObjectStreamer(Context, std::move(MAB), std::move(OW),
                        std::move(Emitter)) {}
 
 bool MCXCOFFStreamer::emitSymbolAttribute(MCSymbol *Sym,
                                           MCSymbolAttr Attribute) {
   auto *Symbol = cast<MCSymbolXCOFF>(Sym);
   getAssembler().registerSymbol(*Symbol);
 
   switch (Attribute) {
   case MCSA_Global:
     Symbol->setStorageClass(XCOFF::C_EXT);
     Symbol->setExternal(true);
     break;
+  case MCSA_LGlobal:
+    Symbol->setStorageClass(XCOFF::C_HIDEXT);
+    Symbol->setExternal(true);
+    break;
   default:
     report_fatal_error("Not implemented yet.");
   }
   return true;
 }
 
 void MCXCOFFStreamer::emitCommonSymbol(MCSymbol *Symbol, uint64_t Size,
                                        unsigned ByteAlignment) {

llvm/lib/MC/XCOFFObjectWriter.cpp

for (const MCSymbol &S : Asm.symbols()) {
     const MCSymbolXCOFF *XSym = cast<MCSymbolXCOFF>(&S);
     const MCSectionXCOFF *ContainingCsect = XSym->getContainingCsect();
 
     if (ContainingCsect->getCSectType() == XCOFF::XTY_ER) {
       // Handle undefined symbol.
       UndefinedCsects.emplace_back(ContainingCsect);
       SectionMap[ContainingCsect] = &UndefinedCsects.back();
     } else {
       // If the symbol is the csect itself, we don't need to put the symbol
       // into csect's Syms.
       if (XSym == ContainingCsect->getQualNameSymbol())
         continue;
 
+      // Only put a label into the symbol table when it is an external label.
+      if (!XSym->isExternal())
+        continue;
+
       assert(SectionMap.find(ContainingCsect) != SectionMap.end() &&
              "Expected containing csect to exist in map");
       // Lookup the containing csect and add the symbol to it.
       SectionMap[ContainingCsect]->Syms.emplace_back(XSym);
     }
 
     // If the name does not fit in the storage provided in the symbol table
     // entry, add it to the string table.

llvm/test/CodeGen/PowerPC/aix-internal.ll

+; RUN: llc -mtriple powerpc-ibm-aix -verify-machineinstrs -mcpu=pwr4  \
+; RUN: -filetype=obj -o %t.o < %s
+; RUN: llvm-readobj --syms %t.o | FileCheck %s
+; RUN: not --crash llc -mtriple powerpc64-ibm-aix -verify-machineinstrs -mcpu=pwr4 \
+; RUN: -filetype=obj -o %t.o < %s 2>&1 | FileCheck --check-prefix=64-CHECK %s
+
+define internal i32 @foo() {
+  ret i32 1
+}
+
+; CHECK:  Symbol {
+; CHECK:       Name: .foo
+; CHECK-NEXT:  Value (RelocatableAddress):
+; CHECK-NEXT:  Section: .text
+; CHECK-NEXT:  Type: 0x0
+; CHECK-NEXT:  StorageClass: C_HIDEXT (0x6B)
+
+; CHECK:  Symbol {
+; CHECK-NEXT: Index: [[#INDX:]]
+; CHECK-NEXT: Name: foo
+; CHECK-NEXT: Value (RelocatableAddress):
+; CHECK-NEXT: Section: .data
+; CHECK-NEXT: Type: 0x0
+; CHECK-NEXT: StorageClass: C_HIDEXT (0x6B)
+; CHECK-NEXT: NumberOfAuxEntries: 1
+; CHECK-NEXT: CSECT Auxiliary Entry {
+; CHECK-NEXT:  Index: [[#INDX+1]]
+; CHECK-NEXT:  SectionLen: 12
+; CHECK-NEXT:  ParameterHashIndex:
+; CHECK-NEXT:  TypeChkSectNum:
+; CHECK-NEXT:  SymbolAlignmentLog2:
+; CHECK-NEXT:  SymbolType: XTY_SD (0x1)
+; CHECK-NEXT:  StorageMappingClass: XMC_DS (0xA)
+
+; Make sure no label is emitted.
+; CHECK-NOT: Name: foo
+
+;64-CHECK: LLVM ERROR: 64-bit XCOFF object files are not supported yet.

llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll

 ;CHECK-NEXT:         .space  1
 
 
 ;CHECKOBJ:      00000000 .text:
 ;CHECKOBJ-NEXT:        0: 38 60 00 00                    li 3, 0
 ;CHECKOBJ-NEXT:        4: 4e 80 00 20                    blr
 ;CHECKOBJ-NEXT:          ...{{[[:space:]] *}}
 ;CHECKOBJ-NEXT: 00000010 .rodata:
-;CHECKOBJ-NEXT:        10: 40 00 00 00                    bdnzf   0, .+0
-;CHECKOBJ-NEXT:        14: 00 00 00 32                    <unknown>
+;CHECKOBJ-NEXT:        10: 40 00 00 00
+;CHECKOBJ-NEXT:        14: 00 00 00 32
 ;CHECKOBJ-NEXT:          ...{{[[:space:]] *}}
-;CHECKOBJ-NEXT: 00000030 .L__const.main.cnst16:
-;CHECKOBJ-NEXT:       30: 40 00 00 00                    bdnzf   0, .+0
-;CHECKOBJ-NEXT:       34: 00 00 00 16                    <unknown>
+;CHECKOBJ-SAME:       30: 40 00 00 00
+;CHECKOBJ-NEXT:       34: 00 00 00 16
 ;CHECKOBJ-NEXT:          ...{{[[:space:]] *}}
-;CHECKOBJ-NEXT: 00000040 .L__const.main.cnst8:
-;CHECKOBJ-NEXT:       40: 40 00 00 08                    bdnzf   0, .+8
-;CHECKOBJ-NEXT:       44: 00 00 00 00                    <unknown>{{[[:space:]] *}}
-;CHECKOBJ-NEXT: 00000048 .L__const.main.cnst4:
-;CHECKOBJ-NEXT:       48: 40 08 00 00                    bdnzf   8, .+0
+;CHECKOBJ-SAME:       40: 40 00 00 08
+;CHECKOBJ-NEXT:       44: 00 00 00 00
+;CHECKOBJ-NEXT:       48: 40 08 00 00
 
 
 ;CHECKSYM:        Symbol {{[{][[:space:]] *}}Index: [[#Index:]]{{[[:space:]] *}}Name: .rodata
 ;CHECKSYM-NEXT:     Value (RelocatableAddress): 0x10
 ;CHECKSYM-NEXT:     Section: .text
 ;CHECKSYM-NEXT:     Type: 0x0
 ;CHECKSYM-NEXT:     StorageClass: C_HIDEXT (0x6B)
 ;CHECKSYM-NEXT:     NumberOfAuxEntries: 1
 ;CHECKSYM-NEXT:     CSECT Auxiliary Entry {
 ;CHECKSYM-NEXT:       Index: [[#Index+1]]
 ;CHECKSYM-NEXT:       SectionLen: 60
 ;CHECKSYM-NEXT:       ParameterHashIndex: 0x0
 ;CHECKSYM-NEXT:       TypeChkSectNum: 0x0
 ;CHECKSYM-NEXT:       SymbolAlignmentLog2: 4
 ;CHECKSYM-NEXT:       SymbolType: XTY_SD (0x1)
 ;CHECKSYM-NEXT:       StorageMappingClass: XMC_RO (0x1)
 ;CHECKSYM-NEXT:       StabInfoIndex: 0x0
 ;CHECKSYM-NEXT:       StabSectNum: 0x0
 ;CHECKSYM-NEXT:     }
 ;CHECKSYM-NEXT:   }
-;CHECKSYM-NEXT:   Symbol {
-;CHECKSYM-NEXT:     Index: [[#Index+2]]
-;CHECKSYM-NEXT:     Name: .L__const.main.cnst32
-;CHECKSYM-NEXT:     Value (RelocatableAddress): 0x10
-;CHECKSYM-NEXT:     Section: .text
-;CHECKSYM-NEXT:     Type: 0x0
-;CHECKSYM-NEXT:     StorageClass: C_HIDEXT (0x6B)
-;CHECKSYM-NEXT:     NumberOfAuxEntries: 1
-;CHECKSYM-NEXT:     CSECT Auxiliary Entry {
-;CHECKSYM-NEXT:       Index: [[#Index+3]]
-;CHECKSYM-NEXT:       ContainingCsectSymbolIndex: [[#Index]]
-;CHECKSYM-NEXT:       ParameterHashIndex: 0x0
-;CHECKSYM-NEXT:       TypeChkSectNum: 0x0
-;CHECKSYM-NEXT:       SymbolAlignmentLog2: 0
-;CHECKSYM-NEXT:       SymbolType: XTY_LD (0x2)
-;CHECKSYM-NEXT:       StorageMappingClass: XMC_RO (0x1)
-;CHECKSYM-NEXT:       StabInfoIndex: 0x0
-;CHECKSYM-NEXT:       StabSectNum: 0x0
-;CHECKSYM-NEXT:     }
-;CHECKSYM-NEXT:   }
-;CHECKSYM-NEXT:   Symbol {
-;CHECKSYM-NEXT:     Index: [[#Index+4]]
-;CHECKSYM-NEXT:     Name: .L__const.main.cnst16
-;CHECKSYM-NEXT:     Value (RelocatableAddress): 0x30
-;CHECKSYM-NEXT:     Section: .text
-;CHECKSYM-NEXT:     Type: 0x0
-;CHECKSYM-NEXT:     StorageClass: C_HIDEXT (0x6B)
-;CHECKSYM-NEXT:     NumberOfAuxEntries: 1
-;CHECKSYM-NEXT:     CSECT Auxiliary Entry {
-;CHECKSYM-NEXT:       Index: [[#Index+5]]
-;CHECKSYM-NEXT:       ContainingCsectSymbolIndex: [[#Index]]
-;CHECKSYM-NEXT:       ParameterHashIndex: 0x0
-;CHECKSYM-NEXT:       TypeChkSectNum: 0x0
-;CHECKSYM-NEXT:       SymbolAlignmentLog2: 0
-;CHECKSYM-NEXT:       SymbolType: XTY_LD (0x2)
-;CHECKSYM-NEXT:       StorageMappingClass: XMC_RO (0x1)
-;CHECKSYM-NEXT:       StabInfoIndex: 0x0
-;CHECKSYM-NEXT:       StabSectNum: 0x0
-;CHECKSYM-NEXT:     }
-;CHECKSYM-NEXT:   }
-;CHECKSYM-NEXT:   Symbol {
-;CHECKSYM-NEXT:     Index: [[#Index+6]]
-;CHECKSYM-NEXT:     Name: .L__const.main.cnst8
-;CHECKSYM-NEXT:     Value (RelocatableAddress): 0x40
-;CHECKSYM-NEXT:     Section: .text
-;CHECKSYM-NEXT:     Type: 0x0
-;CHECKSYM-NEXT:     StorageClass: C_HIDEXT (0x6B)
-;CHECKSYM-NEXT:     NumberOfAuxEntries: 1
-;CHECKSYM-NEXT:     CSECT Auxiliary Entry {
-;CHECKSYM-NEXT:       Index: [[#Index+7]]
-;CHECKSYM-NEXT:       ContainingCsectSymbolIndex: [[#Index]]
-;CHECKSYM-NEXT:       ParameterHashIndex: 0x0
-;CHECKSYM-NEXT:       TypeChkSectNum: 0x0
-;CHECKSYM-NEXT:       SymbolAlignmentLog2: 0
-;CHECKSYM-NEXT:       SymbolType: XTY_LD (0x2)
-;CHECKSYM-NEXT:       StorageMappingClass: XMC_RO (0x1)
-;CHECKSYM-NEXT:       StabInfoIndex: 0x0
-;CHECKSYM-NEXT:       StabSectNum: 0x0
-;CHECKSYM-NEXT:     }
-;CHECKSYM-NEXT:   }
-;CHECKSYM-NEXT:   Symbol {
-;CHECKSYM-NEXT:     Index: [[#Index+8]]
-;CHECKSYM-NEXT:     Name: .L__const.main.cnst4
-;CHECKSYM-NEXT:     Value (RelocatableAddress): 0x48
-;CHECKSYM-NEXT:     Section: .text
-;CHECKSYM-NEXT:     Type: 0x0
-;CHECKSYM-NEXT:     StorageClass: C_HIDEXT (0x6B)
-;CHECKSYM-NEXT:     NumberOfAuxEntries: 1
-;CHECKSYM-NEXT:     CSECT Auxiliary Entry {
-;CHECKSYM-NEXT:       Index: [[#Index+9]]
-;CHECKSYM-NEXT:       ContainingCsectSymbolIndex: [[#Index]]
-;CHECKSYM-NEXT:       ParameterHashIndex: 0x0
-;CHECKSYM-NEXT:       TypeChkSectNum: 0x0
-;CHECKSYM-NEXT:       SymbolAlignmentLog2: 0
-;CHECKSYM-NEXT:       SymbolType: XTY_LD (0x2)
-;CHECKSYM-NEXT:       StorageMappingClass: XMC_RO (0x1)
-;CHECKSYM-NEXT:       StabInfoIndex: 0x0
-;CHECKSYM-NEXT:       StabSectNum: 0x0
-;CHECKSYM-NEXT:     }
-;CHECKSYM-NEXT:   }

llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-str.ll

 ; CHECKOBJ-NEXT:       18: 00 00 01 d0
 ; CHECKOBJ-NEXT:       1c: 00 00 01 d8
 ; CHECKOBJ-NEXT:       20: 00 00 01 9d
 ; CHECKOBJ-NEXT:       24: 00 00 00 00    {{.*}}{{[[:space:]] *}}
 ; CHECKOBJ-NEXT: 00000028 .rodata.str1.1:
 ; CHECKOBJ-NEXT:       28: 68 65 6c 6c
 ; CHECKOBJ-NEXT:       2c: 6f 20 77 6f
 ; CHECKOBJ-NEXT:       30: 72 6c 64 21
-; CHECKOBJ-NEXT:       34: 0a 00 61 62    {{.*}}{{[[:space:]] *}}
-; CHECKOBJ-NEXT: 00000036 .L.str:
-; CHECKOBJ-NEXT:       36: 61 62 63 64
-; CHECKOBJ-NEXT:       3a: 65 66 67 68
-; CHECKOBJ-NEXT:       3e: 00
-; CHECKOBJ-NEXT:       3f: 00
+; CHECKOBJ-NEXT:       34: 0a 00 61 62
+; CHECKOBJ-NEXT:       38: 63 64 65 66
+; CHECKOBJ-NEXT:       3c: 67 68 00 00