This is an archive of the discontinued LLVM Phabricator instance.

Differential D57192

simplify COFF module assembly test and move it to Object
ClosedPublic

Authored by inglorion on Jan 24 2019, 2:08 PM.

Details

Reviewers
pcc
rnk
Commits
rG6710cc7db538: simplify COFF module assembly test and move it to Object
rL352142: simplify COFF module assembly test and move it to Object

Diff Detail

Repository
rL LLVM

Event Timeline

inglorion created this revision.Jan 24 2019, 2:08 PM
Harbormaster completed remote builds in B27268: Diff 183395.Jan 24 2019, 2:08 PM
inglorion mentioned this in D57073: allow COFF .def directive in module assembly when using ThinLTO.Jan 24 2019, 2:09 PM
pcc added inline comments.Jan 24 2019, 2:29 PM
llvm/test/Object/X86/coff-asm.ll
3–4 ↗(On Diff #183395)

Don't you just need these two lines to test your change? The change makes it so that llvm-nm doesn't crash on this input, right?

inglorion marked an inline comment as done.Jan 24 2019, 3:21 PM
inglorion added inline comments.
llvm/test/Object/X86/coff-asm.ll
3–4 ↗(On Diff #183395)

llvm-nm doesn't crash here (or in the next invocation, for that matter) even without D57073. Does it use RecordStreamer? My impression was the crashing code path is only invoked when reading in module assembly in ModuleSymbolTable.

pcc added inline comments.Jan 24 2019, 3:39 PM
llvm/test/Object/X86/coff-asm.ll
3–4 ↗(On Diff #183395)

It should be using RecordStreamer via IRObjectFile.

My mistake, I didn't notice that you were calling llc here. You should be able to reproduce the problem by running llvm-nm on the file that you're calling %t.thinlto.bc here.

inglorion updated this revision to Diff 183428.Jan 24 2019, 4:01 PM

Simpler test. This crashes without D57073 and passes with D57073.

Herald added a subscriber: hiraditya. · View Herald TranscriptJan 24 2019, 4:01 PM
inglorion updated this revision to Diff 183429.Jan 24 2019, 4:02 PM

Re-upload only the test change.

Harbormaster completed remote builds in B27277: Diff 183428.Jan 24 2019, 4:03 PM
Harbormaster completed remote builds in B27278: Diff 183429.
pcc accepted this revision.Jan 24 2019, 4:13 PM

LGTM, thanks!

This revision is now accepted and ready to land.Jan 24 2019, 4:13 PM
inglorion added a comment.Jan 24 2019, 4:32 PM

Thank you!

Closed by commit rL352142: simplify COFF module assembly test and move it to Object (authored by inglorion). · Explain WhyJan 24 2019, 4:33 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
trunk/
test/
MC/
COFF/
21 lines
Object/
X86/
13 lines

Diff 183440

llvm/trunk/test/MC/COFF/module-asm-coff.ll

; Tests COFF-specific directives in module level assembly.
; RUN: llc -filetype=obj %s -o %t.obj
; RUN: llvm-readobj -t %t.obj | FileCheck %s
; RUN: opt -thinlto-bc %s -o %t.thinlto.bc
; RUN: llvm-lto2 run %t.thinlto.bc -o %t.thinlto.obj -r=%t.thinlto.bc,foo,plx
; RUN: llvm-readobj -t %t.thinlto.obj.1 | FileCheck %s
target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-pc-windows-msvc19.0.24210"
module asm ".text"
module asm ".def foo; .scl 3; .type 32; .endef"
module asm ".global foo"
module asm "foo:"
module asm " ret"
; CHECK: Symbol {
; CHECK: Name: foo
; CHECK: StorageClass:
; CHECK-SAME: Static (0x3)

llvm/trunk/test/Object/X86/coff-asm.ll

; Check that llvm-nm doesn't crash on COFF-specific assembly directives
; (PR36789).
; RUN: llvm-as %s -o %t.bc
; RUN: llvm-nm %t.bc
target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-pc-windows-msvc19.0.24210"
module asm ".text"
module asm ".def foo; .scl 2; .type 32; .endef"
module asm "foo:"
module asm " ret"