This is an archive of the discontinued LLVM Phabricator instance.

Object: Fix handling of @@@ in .symver directive
ClosedPublic

Authored by vitalybuka on Mar 8 2018, 2:30 PM.

Details

Summary

name@@@nodename is going to be replaced with name@@nodename if symbols is
defined in the assembled file, or name@nodename if undefined.
https://sourceware.org/binutils/docs/as/Symver.html

Fixes PR36623

Diff Detail

Event Timeline

vitalybuka created this revision.Mar 8 2018, 2:30 PM
vitalybuka added a subscriber: llvm-commits.
vitalybuka added inline comments.
llvm/lib/Object/RecordStreamer.cpp
147

Not sure if I need to lookup in MangledNameMap

espindola edited reviewers, added: espindola; removed: rafael.Mar 8 2018, 3:12 PM
espindola added inline comments.Mar 8 2018, 3:16 PM
llvm/lib/Object/RecordStreamer.cpp
125–126

Use an explicit type instead of auto.

125–126

What version are you using? I don't see createELFSymverAlias on master.

vitalybuka added inline comments.Mar 8 2018, 3:33 PM
llvm/lib/Object/RecordStreamer.cpp
125–126

I moved some code in D44276

vitalybuka updated this revision to Diff 137664.Mar 8 2018, 3:36 PM

auto -> explicit types

vitalybuka marked 2 inline comments as done.Mar 8 2018, 3:37 PM
pcc added inline comments.Mar 9 2018, 11:36 AM
llvm/lib/Object/RecordStreamer.cpp
130

I think you need to defer this processing until the end to handle the case where a .symver appears before an inline asm definition.

155

This line should probably be IsDefined = GV && !GV->isDeclarationForLinker().

clang-format

vitalybuka updated this revision to Diff 137857.Mar 9 2018, 3:44 PM
vitalybuka marked 3 inline comments as done.

Deferred processing.

vitalybuka marked an inline comment as done.Mar 9 2018, 3:44 PM

LGTM

Thanks,
Rafael

Vitaly Buka via Phabricator via llvm-commits
<llvm-commits@lists.llvm.org> writes:

vitalybuka updated this revision to Diff 137861.
vitalybuka added a comment.

fix

https://reviews.llvm.org/D44274

Files:

llvm/lib/Object/RecordStreamer.cpp
llvm/test/LTO/X86/symver-asm3.ll

Index: llvm/test/LTO/X86/symver-asm3.ll

  • /dev/null

+++ llvm/test/LTO/X86/symver-asm3.ll
@@ -0,0 +1,21 @@
+; Test special handling of @@@.
+
+; RUN: llvm-as < %s >%t1
+; RUN: llvm-nm %t1 | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+module asm "_start1:"
+module asm ".symver _start1, foo@@@SOME_VERSION1"
+module asm ".symver _start2, foo@@@SOME_VERSION2"
+module asm ".symver _start3, foo@@@SOME_VERSION3"
+module asm "_start3:"
+module asm ".local _start1"
+module asm ".local _start3"
+
+; CHECK-DAG: t _start1
+; CHECK-DAG: U _start2
+; CHECK-DAG: t foo@@SOME_VERSION1
+; CHECK-DAG: t foo@SOME_VERSION2
+; CHECK-DAG: t foo@@SOME_VERSION3

Index: llvm/lib/Object/RecordStreamer.cpp

  • llvm/lib/Object/RecordStreamer.cpp

+++ llvm/lib/Object/RecordStreamer.cpp
@@ -149,6 +149,7 @@

for (auto &Symver : SymverAliasMap) {
  const MCSymbol *Aliasee = Symver.first;
  MCSymbolAttr Attr = MCSA_Invalid;

+ bool IsDefined = false;

// First check if the aliasee binding was recorded in the asm.
RecordStreamer::State state = getSymbolState(Aliasee);

@@ -165,26 +166,53 @@

  break;
}
  • // If we don't have a symbol attribute from assembly, then check if
  • // the aliasee was defined in the IR.
  • if (Attr == MCSA_Invalid) {
  • const auto *GV = M.getNamedValue(Aliasee->getName());

+ switch (state) {
+ case RecordStreamer::Defined:
+ case RecordStreamer::DefinedGlobal:
+ case RecordStreamer::DefinedWeak:
+ IsDefined = true;
+ break;
+ case RecordStreamer::NeverSeen:
+ case RecordStreamer::Global:
+ case RecordStreamer::Used:
+ case RecordStreamer::UndefinedWeak:
+ break;
+ }
+
+ const GlobalValue *GV = nullptr;
+ if (Attr == MCSA_Invalid || !IsDefined) {
+ GV = M.getNamedValue(Aliasee->getName());

if (!GV) {
  auto MI = MangledNameMap.find(Aliasee->getName());
  if (MI != MangledNameMap.end())
    GV = MI->second;
}
if (GV) {
  • if (GV->hasExternalLinkage())
  • Attr = MCSA_Global;
  • else if (GV->hasLocalLinkage())
  • Attr = MCSA_Local;
  • else if (GV->isWeakForLinker())
  • Attr = MCSA_Weak;

+ If we don't have a symbol attribute from assembly, then check if
+
the aliasee was defined in the IR.
+ if (Attr == MCSA_Invalid) {
+ if (GV->hasExternalLinkage())
+ Attr = MCSA_Global;
+ else if (GV->hasLocalLinkage())
+ Attr = MCSA_Local;
+ else if (GV->isWeakForLinker())
+ Attr = MCSA_Weak;
+ }
+ IsDefined = IsDefined || !GV->isDeclarationForLinker();

  }
}

+

// Set the detected binding on each alias with this aliasee.
for (auto AliasName : Symver.second) {

+ std::pair<StringRef, StringRef> Split = AliasName.split("@@@");
+ SmallString<128> NewName;
+ if (!Split.second.empty() && !Split.second.startswith("@")) {
+ Special processing for "@@@" according
+
https://sourceware.org/binutils/docs/as/Symver.html
+ const char *Separator = IsDefined ? "@@" : "@";
+ AliasName =
+ (Split.first + Separator + Split.second).toStringRef(NewName);
+ }

MCSymbol *Alias = getContext().getOrCreateSymbol(AliasName);
// TODO: Handle "@@@". Depending on SymbolAttribute value it needs to be
// converted into @ or @@.

llvm-commits mailing list
llvm-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

Peter, could you please take a look?

pcc added inline comments.Mar 16 2018, 4:05 PM
llvm/test/LTO/X86/symver-asm3.ll
9

I'd also add a test where the definition is in IR.

21

Is this right? I thought it should be U foo@SOME_VERSION2.

vitalybuka marked an inline comment as done.

added IR test

vitalybuka added inline comments.Mar 16 2018, 5:28 PM
llvm/test/LTO/X86/symver-asm3.ll
21

It should be, but I don't change this behavior. That's how EmitAssignment is already implemented.

I'll can try to fix this in another patch.

pcc accepted this revision.Mar 16 2018, 5:32 PM

LGTM

This revision is now accepted and ready to land.Mar 16 2018, 5:32 PM
This revision was automatically updated to reflect the committed changes.