-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[GOFF] Emit symbols for functions. #144437
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-mc @llvm/pr-subscribers-backend-systemz Author: Kai Nacke (redstar) ChangesA function entry is mapped to a LD symbol with an offset to the begin of the section. Full diff: https://github.com/llvm/llvm-project/pull/144437.diff 7 Files Affected:
diff --git a/llvm/include/llvm/MC/MCGOFFStreamer.h b/llvm/include/llvm/MC/MCGOFFStreamer.h
index 366d7dc08c679..968bef044d175 100644
--- a/llvm/include/llvm/MC/MCGOFFStreamer.h
+++ b/llvm/include/llvm/MC/MCGOFFStreamer.h
@@ -30,9 +30,10 @@ class MCGOFFStreamer : public MCObjectStreamer {
GOFFObjectWriter &getWriter();
- bool emitSymbolAttribute(MCSymbol *Symbol, MCSymbolAttr Attribute) override {
- return false;
- }
+ void emitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc()) override;
+
+ bool emitSymbolAttribute(MCSymbol *Symbol, MCSymbolAttr Attribute) override;
+
void emitCommonSymbol(MCSymbol *Symbol, uint64_t Size,
Align ByteAlignment) override {}
void emitInstToData(const MCInst &Inst, const MCSubtargetInfo &) override {}
diff --git a/llvm/include/llvm/MC/MCSymbolGOFF.h b/llvm/include/llvm/MC/MCSymbolGOFF.h
index d8c570d2de240..b9295d3690803 100644
--- a/llvm/include/llvm/MC/MCSymbolGOFF.h
+++ b/llvm/include/llvm/MC/MCSymbolGOFF.h
@@ -28,7 +28,10 @@ class MCSymbolGOFF : public MCSymbol {
GOFF::LDAttr LDAttributes;
enum SymbolFlags : uint16_t {
- SF_LD = 0x01, // LD attributes are set.
+ SF_LD = 0x01, // LD attributes are set.
+ // Leave place for EX attributes.
+ SF_Hidden = 0x04, // Symbol is hidden, aka not exported.
+ SF_Weak = 0x08, // Symbol is weak.
};
public:
@@ -39,7 +42,8 @@ class MCSymbolGOFF : public MCSymbol {
modifyFlags(SF_LD, SF_LD);
LDAttributes = Attr;
}
- GOFF::LDAttr getLDAttributes() const { return LDAttributes; }
+ const GOFF::LDAttr &getLDAttributes() const { return LDAttributes; }
+ GOFF::LDAttr &getLDAttributes() { return LDAttributes; }
bool hasLDAttributes() const { return getFlags() & SF_LD; }
void setADA(MCSectionGOFF *AssociatedDataArea) {
@@ -48,6 +52,17 @@ class MCSymbolGOFF : public MCSymbol {
}
MCSectionGOFF *getADA() const { return ADA; }
+ void setHidden(bool Value = true) {
+ modifyFlags(Value ? SF_Hidden : 0, SF_Hidden);
+ }
+ bool isHidden() const { return getFlags() & SF_Hidden; }
+ bool isExported() const { return !isHidden(); }
+
+ void setWeak(bool Value = true) { modifyFlags(Value ? SF_Weak : 0, SF_Weak); }
+ bool isWeak() const { return getFlags() & SF_Weak; }
+
+ void initAttributes();
+
static bool classof(const MCSymbol *S) { return S->isGOFF(); }
};
} // end namespace llvm
diff --git a/llvm/lib/MC/CMakeLists.txt b/llvm/lib/MC/CMakeLists.txt
index d662c42c522fc..85e857d3fb406 100644
--- a/llvm/lib/MC/CMakeLists.txt
+++ b/llvm/lib/MC/CMakeLists.txt
@@ -55,6 +55,7 @@ add_llvm_component_library(LLVMMC
MCSubtargetInfo.cpp
MCSymbol.cpp
MCSymbolELF.cpp
+ MCSymbolGOFF.cpp
MCSymbolXCOFF.cpp
MCTargetOptions.cpp
MCTargetOptionsCommandFlags.cpp
diff --git a/llvm/lib/MC/GOFFObjectWriter.cpp b/llvm/lib/MC/GOFFObjectWriter.cpp
index 214533b99688e..c7fa2e99f6625 100644
--- a/llvm/lib/MC/GOFFObjectWriter.cpp
+++ b/llvm/lib/MC/GOFFObjectWriter.cpp
@@ -329,6 +329,7 @@ void GOFFWriter::defineLabel(const MCSymbolGOFF &Symbol) {
Section.getEDAttributes().NameSpace, Symbol.getLDAttributes());
if (Symbol.getADA())
LD.ADAEsdId = Symbol.getADA()->getOrdinal();
+ LD.Offset = Asm.getSymbolOffset(Symbol);
writeSymbol(LD);
}
diff --git a/llvm/lib/MC/MCGOFFStreamer.cpp b/llvm/lib/MC/MCGOFFStreamer.cpp
index b7021915e7b70..451acf3b5d781 100644
--- a/llvm/lib/MC/MCGOFFStreamer.cpp
+++ b/llvm/lib/MC/MCGOFFStreamer.cpp
@@ -15,8 +15,11 @@
#include "llvm/MC/MCAssembler.h"
#include "llvm/MC/MCCodeEmitter.h"
#include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCDirectives.h"
#include "llvm/MC/MCGOFFObjectWriter.h"
+#include "llvm/MC/MCSymbolGOFF.h"
#include "llvm/MC/TargetRegistry.h"
+#include "llvm/Support/Casting.h"
using namespace llvm;
@@ -41,6 +44,61 @@ void MCGOFFStreamer::changeSection(MCSection *Section, uint32_t Subsection) {
MCObjectStreamer::changeSection(Section, Subsection);
}
+void MCGOFFStreamer::emitLabel(MCSymbol *Symbol, SMLoc Loc) {
+ MCObjectStreamer::emitLabel(Symbol, Loc);
+ cast<MCSymbolGOFF>(Symbol)->initAttributes();
+}
+
+bool MCGOFFStreamer::emitSymbolAttribute(MCSymbol *Sym,
+ MCSymbolAttr Attribute) {
+ auto *Symbol = cast<MCSymbolGOFF>(Sym);
+ switch (Attribute) {
+ case MCSA_Invalid:
+ case MCSA_Cold:
+ case MCSA_ELF_TypeFunction:
+ case MCSA_ELF_TypeIndFunction:
+ case MCSA_ELF_TypeObject:
+ case MCSA_ELF_TypeTLS:
+ case MCSA_ELF_TypeCommon:
+ case MCSA_ELF_TypeNoType:
+ case MCSA_ELF_TypeGnuUniqueObject:
+ case MCSA_LGlobal:
+ case MCSA_Extern:
+ case MCSA_Exported:
+ case MCSA_IndirectSymbol:
+ case MCSA_Internal:
+ case MCSA_LazyReference:
+ case MCSA_NoDeadStrip:
+ case MCSA_SymbolResolver:
+ case MCSA_AltEntry:
+ case MCSA_PrivateExtern:
+ case MCSA_Protected:
+ case MCSA_Reference:
+ case MCSA_WeakDefinition:
+ case MCSA_WeakDefAutoPrivate:
+ case MCSA_WeakAntiDep:
+ case MCSA_Memtag:
+ return false;
+
+ case MCSA_Global:
+ Symbol->setExternal(true);
+ break;
+ case MCSA_Local:
+ Symbol->setExternal(false);
+ break;
+ case MCSA_Weak:
+ case MCSA_WeakReference:
+ Symbol->setExternal(true);
+ Symbol->setWeak();
+ break;
+ case MCSA_Hidden:
+ Symbol->setHidden(true);
+ break;
+ }
+
+ return true;
+}
+
MCStreamer *llvm::createGOFFStreamer(MCContext &Context,
std::unique_ptr<MCAsmBackend> &&MAB,
std::unique_ptr<MCObjectWriter> &&OW,
diff --git a/llvm/lib/MC/MCSymbolGOFF.cpp b/llvm/lib/MC/MCSymbolGOFF.cpp
new file mode 100644
index 0000000000000..6d07d6a54a233
--- /dev/null
+++ b/llvm/lib/MC/MCSymbolGOFF.cpp
@@ -0,0 +1,38 @@
+//===- MCSymbolGOFF.cpp - GOFF Symbol Representation ----------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/MC/MCSymbolGOFF.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/ErrorHandling.h"
+
+using namespace llvm;
+
+void MCSymbolGOFF::initAttributes() {
+ if (hasLDAttributes())
+ return;
+
+ if (isDefined()) {
+ MCSectionGOFF &Section = cast<MCSectionGOFF>(getSection());
+ GOFF::ESDBindingScope BindingScope =
+ isExternal() ? (isExported() ? GOFF::ESD_BSC_ImportExport
+ : GOFF::ESD_BSC_Library)
+ : GOFF::ESD_BSC_Section;
+ GOFF::ESDBindingStrength BindingStrength =
+ isWeak() ? GOFF::ESDBindingStrength::ESD_BST_Weak
+ : GOFF::ESDBindingStrength::ESD_BST_Strong;
+ if (Section.isED()) {
+ setLDAttributes(GOFF::LDAttr{false, GOFF::ESD_EXE_CODE, BindingStrength,
+ GOFF::LINKAGE, GOFF::AMODE, BindingScope});
+ } else if (Section.isPR()) {
+ // For data symbols, the attributes are already determind in TLOFI.
+ // TODO Does it make sense to it to here?
+ } else
+ llvm_unreachable("Unexpected section type for label");
+ }
+ // TODO Handle external symbol.
+}
diff --git a/llvm/test/CodeGen/SystemZ/zos-section-1.ll b/llvm/test/CodeGen/SystemZ/zos-section-1.ll
index b98584df54d5a..6caa8f4d607de 100644
--- a/llvm/test/CodeGen/SystemZ/zos-section-1.ll
+++ b/llvm/test/CodeGen/SystemZ/zos-section-1.ll
@@ -104,26 +104,34 @@ entry:
; CHECK-NEXT: 000300 00 00 00 00 00 00 00 00 00 00 00 00 04 00 00 02
; CHECK-NEXT: 000310 00 01 20 00 00 00 00 06 a3 85 a2 a3 7b c3 00 00
+; ESD record, type LD.
+; The name is me.
+; CHECK-NEXT: 000320 03 00 00 02 [[ME:00 00 00 09]] [[C_CODE64]] 00 00 00 00
+; CHECK-NEXT: 000330 00 00 00 10 00 00 00 00 00 00 00 00 00 00 00 00
+; CHECK-NEXT: 000340 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00
+; CHECK-NEXT: 000350 00 00 00 00 00 00 00 00 00 00 00 00 04 00 00 02
+; CHECK-NEXT: 000360 00 04 20 00 00 00 00 02 94 85 00 00 00 00 00 00
+
; Text record for the code section C_CODE64.
; The regular expression matches the lower byte of the length.
-; CHECK-NEXT: 000320 03 11 00 00 [[C_CODE64]] 00 00 00 00 00 00 00 00
-; CHECK-NEXT: 000330 00 00 00 00 00 00 00 {{..}} 00 c3 00 c5 00 c5 00 f1
+; CHECK-NEXT: 000370 03 11 00 00 [[C_CODE64]] 00 00 00 00 00 00 00 00
+; CHECK-NEXT: 000380 00 00 00 00 00 00 00 {{..}} 00 c3 00 c5 00 c5 00 f1
; Text record for the section .&ppa2.
-; CHECK: 0003c0 03 10 00 00 [[PPA2]] 00 00 00 00 00 00 00 00
-; CHECK-NEXT: 0003d0 00 00 00 00 00 00 00 {{..}} {{.*}}
+; CHECK: 000410 03 10 00 00 [[PPA2]] 00 00 00 00 00 00 00 00
+; CHECK-NEXT: 000420 00 00 00 00 00 00 00 {{..}} {{.*}}
; Text record for the ADA section test#S.
-; CHECK: 000410 03 10 00 00 [[TESTS]] 00 00 00 00 00 00 00 00
-; CHECK-NEXT: 000420 00 00 00 00 00 00 00 {{..}} {{.*}}
+; CHECK: 000460 03 10 00 00 [[TESTS]] 00 00 00 00 00 00 00 00
+; CHECK-NEXT: 000470 00 00 00 00 00 00 00 {{..}} {{.*}}
; Text record for the section B_IDRL.
-; CHECK: 000460 03 10 00 01 [[BIDRL]] 00 00 00 00 00 00 00 00
-; CHECK-NEXT: 000470 00 00 00 00 00 00 00 {{..}} {{.*}}
+; CHECK: 0004b0 03 10 00 01 [[BIDRL]] 00 00 00 00 00 00 00 00
+; CHECK-NEXT: 0004c0 00 00 00 00 00 00 00 {{..}} {{.*}}
; End record.
-; CHECK: 0004b0 03 40 00 00 00 00 00 00 00 00 00 00 00 00 00 00
-; CHECK-NEXT: 0004c0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
-; CHECK-NEXT: 0004d0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
-; CHECK-NEXT: 0004e0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
-; CHECK-NEXT: 0004f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+; CHECK: 000500 03 40 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+; CHECK-NEXT: 000510 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+; CHECK-NEXT: 000520 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+; CHECK-NEXT: 000530 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+; CHECK-NEXT: 000540 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
|
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp,h -- llvm/lib/MC/MCSymbolGOFF.cpp llvm/include/llvm/MC/MCDirectives.h llvm/include/llvm/MC/MCGOFFAttributes.h llvm/include/llvm/MC/MCGOFFObjectWriter.h llvm/include/llvm/MC/MCGOFFStreamer.h llvm/include/llvm/MC/MCSymbolGOFF.h llvm/lib/MC/GOFFObjectWriter.cpp llvm/lib/MC/MCAsmStreamer.cpp llvm/lib/MC/MCELFStreamer.cpp llvm/lib/MC/MCGOFFStreamer.cpp llvm/lib/MC/MCMachOStreamer.cpp llvm/lib/Target/SystemZ/MCTargetDesc/SystemZHLASMAsmStreamer.cpp llvm/lib/Target/SystemZ/MCTargetDesc/SystemZHLASMAsmStreamer.h llvm/lib/Target/SystemZ/MCTargetDesc/SystemZTargetStreamer.cpp llvm/lib/Target/SystemZ/MCTargetDesc/SystemZTargetStreamer.h llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp --diff_from_common_commit
View the diff from clang-format here.diff --git a/llvm/lib/MC/GOFFObjectWriter.cpp b/llvm/lib/MC/GOFFObjectWriter.cpp
index 860d0a50c..2e1bc4993 100644
--- a/llvm/lib/MC/GOFFObjectWriter.cpp
+++ b/llvm/lib/MC/GOFFObjectWriter.cpp
@@ -348,8 +348,8 @@ void GOFFWriter::defineLabel(const MCSymbolGOFF &Symbol) {
}
void GOFFWriter::defineExtern(const MCSymbolGOFF &Symbol) {
- GOFFSymbol ER(Symbol.getName(), Symbol.getIndex(),
- RootSD->getOrdinal(), Symbol.getERAttributes());
+ GOFFSymbol ER(Symbol.getName(), Symbol.getIndex(), RootSD->getOrdinal(),
+ Symbol.getERAttributes());
writeSymbol(ER);
}
|
MaskRay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! But I will delegated to a GOFF expert for approval...
| Symbol->setExternal(true); | ||
| break; | ||
| case MCSA_Local: | ||
| Symbol->setExternal(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is local/global/weak tested?
uweigand
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There doesn't appear to be any asm output for these?
The current branch base does not include the HLASM infrastructure from Tony. I commit the other approved PRs, then I can rebase this one on latest main, and add the asm output.
I add those tests when I add the assembly output. |
62c4a6c to
78356cd
Compare
f20a8fa to
72d36e3
Compare
7763a9f to
fcd26b0
Compare
0ed7c36 to
069b0a5
Compare
A function entry is mapped to a LD symbol with an offset to the begin of the section.
Adds HLASM output and tests for it, per reviewer comment. Also adds external references, because it fits very well into the implementation.
069b0a5 to
c19957c
Compare
llvm/include/llvm/MC/MCDirectives.h
Outdated
| MCSA_WeakAntiDep, ///< .weak_anti_dep (COFF) | ||
| MCSA_Memtag, ///< .memtag (ELF) | ||
| MCSA_Code, ///< symbol is code (GOFF) | ||
| MCSA_Data, ///< symbol is data (GOFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above two duplicate the semantics of MCSA_ELF_TypeFunction and MCSA_ELF_TypeObject in a sense. I'm wondering if we can hook into that (already existing) common code logic ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed, now using MCSA_ELF_TypeFunction / MCSA_ELF_TypeObject. It's indeed very similar.
| SF_ER = 0x02, // ER attributes are set. | ||
| SF_Hidden = 0x04, // Symbol is hidden, aka not exported. | ||
| SF_Weak = 0x08, // Symbol is weak. | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above fields CodeData and Linkage as well as the new flags SF_Hidden and SF_Weak duplicate information already present in the LDAttributes or ERAttributes field. This should be avoided - what does it mean if the two places disagree?
Not sure what the right fix is; either we can create and maintain the proper attribute structure at all times, or if not, maybe we should not have the attributes in MCSymbolGOFF at all (only those other fields), and simply generate the proper attributes structure only where it is needed to write out the symbol.
| } | ||
| GOFF::LDAttr getLDAttributes() const { return LDAttributes; } | ||
| const GOFF::LDAttr &getLDAttributes() const { return LDAttributes; } | ||
| GOFF::LDAttr &getLDAttributes() { return LDAttributes; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a non-const getter at all here?
llvm/include/llvm/MC/MCSymbolGOFF.h
Outdated
| // Owner of the symbol if symbol is an external reference. External references | ||
| // need a section, too, but adding them to a section would make the symbol | ||
| // defined. | ||
| MCSectionGOFF *Owner = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can see, all external reference symbols share the same owner, right? It seems a bit odd to duplicate this pointer into each and every symbol then ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the code to push the owner (aka the Root SD section) into the GOFFObjectWriter.
|
|
||
| using namespace llvm; | ||
|
|
||
| void MCSymbolGOFF::initAttributes() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on how we resolve the attribute question above, I don't think we should have this function at all. Either attributes are set up initially at symbol creation and modified on the fly as needed, or else the information is directly taken form flags and other fields at the place where it is needed.
| } | ||
|
|
||
| bool SystemZHLASMAsmStreamer::emitSymbolAttribute(MCSymbol *Sym, | ||
| MCSymbolAttr Attribute) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a bit odd the the asm version of emitSymbolAttribute doesn't actually emit any assembler, but just does the same thing as the object file version. That may be unavoidable as HLASM doesn't have distinct directives but you have to group everything into a single XATTR. But then maybe the two streamers should actually share the implementation somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the AsmStreamer doesn't do anything, you can remove these symbol property adjustment - making this function a no-op.
| emitIDRLSection(M); | ||
| // Emit EXTRN declarations. | ||
| OutStreamer->pushSection(); | ||
| for (auto &GO : M.global_objects()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit unfortunate if we have to have this loop here. It looks like AsmPrinter::doFinalization is already doing very similar things for other platforms, might be nicer if we could hook into that.
| } | ||
| OutStreamer->switchSection( | ||
| static_cast<MCSectionGOFF *>(getObjFileLowering().getTextSection()) | ||
| ->getParent()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole section switching stuff is needed only to set those symbol owners (all to the same section), right? Maybe we can find a better way to handle this.
| @@ -1,4 +1,4 @@ | |||
| ; RUN: llc -mtriple s390x-ibm-zos < %s | FileCheck %s | |||
| ; RUN: llc -mtriple s390x-ibm-zos -emit-gnuas-syntax-on-zos=false < %s | FileCheck %s | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate topic, but when do you think we can switch to HLASM syntax by default for all tests?
Use MCSA_ELF_TypeFunction/TypeObject instead.
🐧 Linux x64 Test Results
|
It's only needed on the binary object path, pushing the section into the GOFFObjectWriter is enough.
| case MCSA_Local: | ||
| Symbol->setExternal(false); | ||
| break; | ||
| case MCSA_Weak: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for .globl; .local; .weak, does the last directive win? MCELFStreamer.cpp reports warnings/errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, currently the last directive wins. I'll check the MCELFStreamer implementation.
| Symbol->setExternal(false); | ||
| break; | ||
| case MCSA_Weak: | ||
| case MCSA_WeakReference: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MCSA_WeakReference is used by AsmPrinter::emitGlobalAlias - ensure that GOFF has such a test.
A function entry is mapped to a LD symbol with an offset to the begin of the section.