This is an archive of the discontinued LLVM Phabricator instance.

[gcov] Move llvm_writeout_files from atexit to a static destructor
ClosedPublic

Authored by MaskRay on Jun 19 2020, 9:26 PM.

Details

Summary

atexit registered functions run earlier so __attribute__((destructor))
annotated functions cannot be tracked.

Set a priority of 100 (compatible with GCC 7 onwards) to track
destructors and destructors whose priorities are greater than 100.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=7970

Diff Detail

Event Timeline

MaskRay created this revision.Jun 19 2020, 9:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2020, 9:26 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
MaskRay edited the summary of this revision. (Show Details)Jun 19 2020, 9:27 PM
calixte accepted this revision.Jun 22 2020, 1:05 AM

LGTM

This revision is now accepted and ready to land.Jun 22 2020, 1:05 AM
marco-c requested changes to this revision.Jun 22 2020, 5:21 AM

There is only a problem with the (possibly very rare) case where calling exec* fails:

  • the process calls exec*;
  • llvm_writeout_files is called, which removes the writeout list;
  • exec* fails;
  • the process continues, the coverage is lost because when it ends there is no writeout list anymore.

For reference, we inject llvm_writeout_files before exec* here: https://github.com/llvm/llvm-project/blob/16cc759ebd56fddd2a9d2437b810ced885ebda73/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp#L684.

This revision now requires changes to proceed.Jun 22 2020, 5:21 AM

There is only a problem with the (possibly very rare) case where calling exec* fails:

  • the process calls exec*;
  • llvm_writeout_files is called, which removes the writeout list;
  • exec* fails;
  • the process continues, the coverage is lost because when it ends there is no writeout list anymore.

For reference, we inject llvm_writeout_files before exec* here: https://github.com/llvm/llvm-project/blob/16cc759ebd56fddd2a9d2437b810ced885ebda73/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp#L684.

How does the patch cause a new problem? This is a pre-existing problem with the previous atexit approach.

It's because you moved fn_list_remove(&writeout_fn_list); in llvm_writeout_files(void)

MaskRay added a comment.EditedJun 22 2020, 12:23 PM

It's because you moved fn_list_remove(&writeout_fn_list); in llvm_writeout_files(void)

Can you elaborate how deleting atexit(llvm_delete_writeout_function_list); can be a problem? Please also propose a solution if the very rare use case fails. You can add a test if you want to demonstrate this is a feature we want to support.

I have to move it because we need to postpone llvm_writeout_files to a later stage. The function (via a static destructor) still runs if exec* fails.

How does the patch cause a new problem? This is a pre-existing problem with the previous atexit approach.

Can you elaborate how deleting atexit(llvm_delete_writeout_function_list); can be a problem?

With the atexit approach, we were not deleting the writeout function list before exec.

With atexit:

  • process calls exec;
  • we write out the files;
  • exec fails;
  • at some point the process ends;
  • we write out the files (so we don't lose coverage information collected between when exec failed and now);
  • we delete the writeout function list.

With static destructor:

  • process calls exec;
  • we write out the files;
  • we delete the writeout function list;
  • exec fails;
  • at some point the process ends;
  • we can't write out the files because the writeout function list was deleted (and so we lose coverage information).

Please also propose a solution if the very rare use case fails. You can add a test if you want to demonstrate this is a feature we want to support.

Aren't these usually responsibility of the author of the proposed change? :)

Anyway, a couple possible solutions:

  1. Don't merge llvm_delete_writeout_function_list into llvm_writeout_files, make llvm_delete_writeout_function_list a static destructor too (making it run after llvm_writeout_files);
  2. Create a new function llvm_writeout_destructor (please find a better name if you do this!), which is a static destructor and calls llvm_writeout_files and then llvm_delete_writeout_function_list. So before exec* we keep calling llvm_writeout_files without deleting the writeout function list.

I personally prefer the second option.

How does the patch cause a new problem? This is a pre-existing problem with the previous atexit approach.

Can you elaborate how deleting atexit(llvm_delete_writeout_function_list); can be a problem?

With the atexit approach, we were not deleting the writeout function list before exec.

With atexit:

  • process calls exec;
  • we write out the files;
  • exec fails;
  • at some point the process ends;
  • we write out the files (so we don't lose coverage information collected between when exec failed and now);
  • we delete the writeout function list.

With static destructor:

  • process calls exec;
  • we write out the files;
  • we delete the writeout function list;
  • exec fails;
  • at some point the process ends;
  • we can't write out the files because the writeout function list was deleted (and so we lose coverage information).

Please also propose a solution if the very rare use case fails. You can add a test if you want to demonstrate this is a feature we want to support.

Aren't these usually responsibility of the author of the proposed change? :)

Anyway, a couple possible solutions:

  1. Don't merge llvm_delete_writeout_function_list into llvm_writeout_files, make llvm_delete_writeout_function_list a static destructor too (making it run after llvm_writeout_files);
  2. Create a new function llvm_writeout_destructor (please find a better name if you do this!), which is a static destructor and calls llvm_writeout_files and then llvm_delete_writeout_function_list. So before exec* we keep calling llvm_writeout_files without deleting the writeout function list.

I personally prefer the second option.

Thanks! I did not notice that llvm_writeout_files could be called by execl, so I failed to notice the issue you reported. (I thought it was an edge case, thus asked you to clarify.)

MaskRay updated this revision to Diff 272600.Jun 22 2020, 9:21 PM

Handle execl and improve test

MaskRay updated this revision to Diff 272601.Jun 22 2020, 9:23 PM

[[@LINE]] -> #@LINE

The former is legacy syntax.

@marco-c Hi Marco, are you happy with the new version?

MaskRay added a comment.EditedJun 29 2020, 10:53 AM

@marco-c Hi Marco, are you happy with the new version?

@marco-c :) (Sorry to bother you again in less than one week. I hope this will not take up much of your time)

marco-c accepted this revision.Jul 1 2020, 1:58 AM

Sorry, I was on holiday.
Yes, this LGTM!

This revision is now accepted and ready to land.Jul 1 2020, 1:58 AM
This revision was automatically updated to reflect the committed changes.