Page MenuHomePhabricator

llvm-nm: Observe -no-llvm-bc for archive members
ClosedPublic

Authored by kastiglione on Jun 29 2018, 11:22 PM.

Details

Summary

This change fixes the -no-llvm-bc flag to work with object files within
archives. Currently the -no-llvm-bc flag works for regular object files, but
not static libraries, where it continues to show bitcode symbol info.

Original support was added in D4371.

Diff Detail

Repository
rL LLVM

Event Timeline

kastiglione created this revision.Jun 29 2018, 11:22 PM

Remove hardcoded bitcode from test

kastiglione added inline comments.Jun 30 2018, 12:30 AM
test/tools/llvm-nm/X86/archive-no-llvm-bc.test
1 ↗(On Diff #153616)

Is it a bad practice to use a known file name at a relative path like this?

kastiglione marked an inline comment as done.Jun 30 2018, 3:56 PM

Use %t instead of %T

compnerd added inline comments.Jul 18 2018, 10:52 AM
test/tools/llvm-nm/X86/archive-no-llvm-bc.test
2 ↗(On Diff #153646)

I dont understand this. Where is embedded.bc being used? It seems that you could completely remove this bit and still have the test work?

smeenai added inline comments.Jul 18 2018, 3:30 PM
test/tools/llvm-nm/X86/archive-no-llvm-bc.test
1 ↗(On Diff #153646)

mkdir -p is probably better.

2 ↗(On Diff #153646)

It's used in the incbin directive below (line 14).

kastiglione added inline comments.Jul 18 2018, 4:35 PM
test/tools/llvm-nm/X86/archive-no-llvm-bc.test
1 ↗(On Diff #153646)

good call

2 ↗(On Diff #153646)

@compnerd the IR in this test is used twice, once to generate bitcode, and a second time to generate an object file. As @smeenai pointed to, there are two module asm directives that embed the bitcode.

Ah, I see. I think I had overlooked the .incbin.

Update lit setup

Fix accidentally changed llvm-nm test invocation

kastiglione marked an inline comment as done.Aug 2 2018, 6:12 PM

@smeenai or @compnerd – much late follow up, but either of you willing to accept?

smeenai resigned from this revision.Nov 5 2018, 11:08 AM

I'm not familiar enough with this part of the code to feel comfortable accepting, sorry.

kastiglione edited the summary of this revision. (Show Details)Nov 27 2018, 7:11 PM

@compnerd would you be willing to accept?

@pcc I have this fix related to -no-llvm-bc (D4371). I'm looking for a reviewer: could you review, or could you suggest someone? Thanks.

compnerd accepted this revision.Feb 3 2019, 4:13 PM

@kastiglione - bleh, seems that this got lost. This seems like a good fix that we should get in.

This revision is now accepted and ready to land.Feb 3 2019, 4:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2019, 4:13 PM
This revision was automatically updated to reflect the committed changes.