This is an archive of the discontinued LLVM Phabricator instance.

Differential D71443

[MC][PowerPC] Fix a crash when redefining a symbol after .set
ClosedPublic

Authored by MaskRay on Dec 12 2019, 4:56 PM.

Details

Reviewers
arichardson
hfinkel
luporl
steven.zhang
Group Reviewers
Restricted Project
Commits
rGf99eedeb7264: [MC][PowerPC] Fix a crash when redefining a symbol after .set
Summary

Fix PR44284.

This is probably not valid assembly but we should not crash.

Diff Detail

Event Timeline

MaskRay created this revision.Dec 12 2019, 4:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2019, 4:56 PM
Herald added subscribers: llvm-commits, shchenz, jsji and 3 others. · View Herald Transcript
Harbormaster completed remote builds in B42435: Diff 233715.Dec 12 2019, 5:00 PM
Herald added a subscriber: • wuzish. · View Herald TranscriptDec 12 2019, 5:00 PM
MaskRay edited the summary of this revision. (Show Details)Dec 12 2019, 5:01 PM
MaskRay added a reviewer: Restricted Project.Dec 12 2019, 9:55 PM
steven.zhang accepted this revision.Dec 13 2019, 12:45 AM
steven.zhang added a subscriber: steven.zhang.

LGTM, thank you for the fixing.

This revision is now accepted and ready to land.Dec 13 2019, 12:45 AM
luporl accepted this revision.Dec 13 2019, 3:15 AM

LGTM. It seems I did not consider that Symbol could change its type between when it was added in emitAssignment() and when processed in finish().

Closed by commit rGf99eedeb7264: [MC][PowerPC] Fix a crash when redefining a symbol after .set (authored by MaskRay). · Explain WhyDec 13 2019, 9:38 AM
This revision was automatically updated to reflect the committed changes.
nemanjai added a comment.Apr 9 2020, 8:46 PM

@MaskRay @luporl Is it possible for Sym to be null here? Do we need a nullptr check? I ask because we seem to have a crash here reported in https://bugs.llvm.org/show_bug.cgi?id=45366 that we cannot reproduce. But I wonder if perhaps this might be the issue.

I have asked the OP of the PR to try the patch that adds the null check, but I was hoping to get your opinion on this as well.

MaskRay added a comment.EditedApr 9 2020, 10:25 PM
In D71443#1973732, @nemanjai wrote:

@MaskRay @luporl Is it possible for Sym to be null here? Do we need a nullptr check? I ask because we seem to have a crash here reported in https://bugs.llvm.org/show_bug.cgi?id=45366 that we cannot reproduce. But I wonder if perhaps this might be the issue.

I have asked the OP of the PR to try the patch that adds the null check, but I was hoping to get your opinion on this as well.

In a -DLLVM_ENABLE_ASSERTIONS=on build, Sym can't be nullptr. Might to good to ask the OP of https://bugs.llvm.org/show_bug.cgi?id=45366 whether this can be reproduced

void emitAssignment(MCSymbol *S, const MCExpr *Value) override {
  auto *Symbol = cast<MCSymbolELF>(S); //////////// asserted to be non-null

  // When encoding an assignment to set symbol A to symbol B, also copy
  // the st_other bits encoding the local entry point offset.
  if (copyLocalEntry(Symbol, Value))
    UpdateOther.insert(Symbol); ///////// only way an element is inserted into UpdateOther
  else
    UpdateOther.erase(Symbol);
}

Here is the only caller of llvm::MCTargetStreamer::emitAssignment

// https://github.com/llvm/llvm-project/blob/master/llvm/lib/MC/MCStreamer.cpp#L966
void MCStreamer::emitAssignment(MCSymbol *Symbol, const MCExpr *Value) {
  visitUsedExpr(*Value);
  Symbol->setVariableValue(Value);

  MCTargetStreamer *TS = getTargetStreamer();
  if (TS)
    TS->emitAssignment(Symbol, Value);  //////////// Symbol can't be nullptr
}

Revision Contents

PathSize
llvm/
lib/
Target/
PowerPC/
MCTargetDesc/
3 lines
test/
MC/
PowerPC/
5 lines

Diff 233824

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp

Show First 20 Lines • Show All 195 Lines • ▼ Show 20 Lines void emitAssignment(MCSymbol *S, const MCExpr *Value) override {
if (copyLocalEntry(Symbol, Value)) if (copyLocalEntry(Symbol, Value))
UpdateOther.insert(Symbol); UpdateOther.insert(Symbol);
else else
UpdateOther.erase(Symbol); UpdateOther.erase(Symbol);
} }
void finish() override { void finish() override {
for (auto *Sym : UpdateOther) for (auto *Sym : UpdateOther)
if (Sym->isVariable())
copyLocalEntry(Sym, Sym->getVariableValue()); copyLocalEntry(Sym, Sym->getVariableValue());
} }
private: private:
SmallPtrSet<MCSymbolELF *, 32> UpdateOther; SmallPtrSet<MCSymbolELF *, 32> UpdateOther;
bool copyLocalEntry(MCSymbolELF *D, const MCExpr *S) { bool copyLocalEntry(MCSymbolELF *D, const MCExpr *S) {
auto *Ref = dyn_cast<const MCSymbolRefExpr>(S); auto *Ref = dyn_cast<const MCSymbolRefExpr>(S);
if (!Ref) if (!Ref)
▲ Show 20 Lines • Show All 115 Lines • Show Last 20 Lines

llvm/test/MC/PowerPC/ppc64-localentry-symbols.s

Show All 26 Lines
.equ weak_func, func .equ weak_func, func
.p2align 2 .p2align 2
.type func,@function .type func,@function
func: func:
nop nop
nop nop
.localentry func, 8 .localentry func, 8
## PR44284 Don't crash if err is redefined after .set
.set err, _err
.globl err
err: