This is an archive of the discontinued LLVM Phabricator instance.

Add zero size dummy data to ensure section symbol is always created
ClosedPublic

Authored by davidxl on Nov 6 2015, 3:47 PM.

Details

Summary

There is a scenario where a user does not specify -fprofile-instr-generate on compilation command but on the linker command only for various reasons. In such cases, since no object is instrumented, the llvm_prof_xxx sections won't exist and linker won't generate __start__.. and _stop__.. symbols leading to linker time unsat.

Making start_../stop_.. weak extern symbols solve the problem when building executable, but PC relative dynamic reloc is not allowed so shared library case will still fail.

THis simple patch fixes the problem. It won't have any effect on regular use cases.

It has another added benefit -- it allows us to conditionally turn off generation of name sections completely (depending on user option) to reduce binary/profile data size -- that feature will come later.

Diff Detail

Event Timeline

davidxl updated this revision to Diff 39604.Nov 6 2015, 3:47 PM
davidxl retitled this revision from to Add zero size dummy data to ensure section symbol is always created.
davidxl updated this object.
davidxl added a reviewer: vsk.
davidxl added a subscriber: llvm-commits.
davidxl updated this revision to Diff 39663.Nov 8 2015, 8:11 PM

Update patch:
do not dump a header when there is no data to be dumped.

vsk edited edge metadata.Nov 9 2015, 3:22 PM

I think the same idea works for InstrProfilingPlatformDarwin.c. Do you mind adding it there?

This could also use a test. Can we compile an input from test/profile without -fprofile-instr-generate, pass it to the linker, and check for the sections we expect? If that requires too many platform-specific tools, is there some other way we can test this?

davidxl updated this revision to Diff 40079.Nov 12 2015, 12:32 PM
davidxl edited edge metadata.

The change does not apply to Darwin as on Darwin, we still pay the overhead of emitting the runtime hook user function and does not use -u,symbol trick -- so when no module is instrumented, the runtime library won't linked in.

An extensive test case is added for shared library PGO -- it covers all possible scenarios -- it is better to have a black box testing without need to check the implementation detail.

Also suppressed the pedantic warning for zero size array.

vsk added a comment.Nov 13 2015, 10:14 AM

Nice!

test/profile/instrprof-shared.test
4 ↗(On Diff #40079)

Could you rename instrprof-shared-lib.o to instrprof-shared-lib-noinstr.o?

7 ↗(On Diff #40079)

Could you add a comment explaining your approach? Something like:

'''
This test produces three shared objects.

  • libt-instr.so is instrumented
  • libt-no-instr1.so is not instrumented
  • libt-no-instr2.so is compiled with instrumentation enabled, but is formed out of an uninstrumented object

Verify that compiling against these shared objects with and without instrumentation enabled behaves as expected.
'''

9 ↗(On Diff #40079)

%t-instr-no-instr -> %t-instr-no-instr1?

12 ↗(On Diff #40079)

%t-no-instr-no-instr -> %t-no-instr-no-instr1?

30 ↗(On Diff #40079)

Can you add negative tests (not RUN?) showing that the uninstrumented binaries produce no profraw files?

davidxl marked 4 inline comments as done.Nov 13 2015, 12:29 PM
davidxl added inline comments.
test/profile/instrprof-shared.test
30 ↗(On Diff #40079)

I have added two. There are two cases no-instr*-no-instr2 where linux version produces zero size prof data vs non-existing file (making changes to unlink file can be tricky) -- so adding a test here can be tricky. As this is not the expected using case anyway, existing testings already cover the expected behavior (build, run etc), I will leave this as it is .

davidxl updated this revision to Diff 40167.Nov 13 2015, 12:30 PM

Updating test case according to vsk's comments.

vsk added a comment.Nov 13 2015, 12:37 PM

Thanks, lgtm

test/profile/instrprof-shared.test
6 ↗(On Diff #40167)

nit: extra 'but is' here

This revision was automatically updated to reflect the committed changes.