This is an archive of the discontinued LLVM Phabricator instance.

[ASan] Linker-initialize static ScopedInErrorReport::current_error_.
ClosedPublic

Authored by alekseyshl on Jun 13 2018, 12:06 PM.

Details

Summary

Static ScopedInErrorReport::current_error_ can be linker initialized to
shave one global ctor call on application startup and be __asan_init-safe.

Global constructors in ASan runtime are bad because __asan_init runs
from preinit_array, before any such constructors.

Issue: https://github.com/google/sanitizers/issues/194

Diff Detail

Repository
rL LLVM

Event Timeline

alekseyshl created this revision.Jun 13 2018, 12:06 PM
Herald added subscribers: Restricted Project, delcypher, kubamracek. · View Herald TranscriptJun 13 2018, 12:06 PM
alekseyshl added a comment.EditedJun 13 2018, 1:10 PM

Isn't this basically https://reviews.llvm.org/D44243 ?

It is! Thanks, missed that one. I'm fine with that one being committed too. Is the explanation on this patch satisfactory?

delcypher added a comment.EditedJun 14 2018, 4:23 AM

Isn't this basically https://reviews.llvm.org/D44243 ?

It is! Thanks, missed that one. I'm fine with that one being committed too. Is the explanation on this patch satisfactory?

minor nit: s/on the startup/on application startup/ or s/on the startup/on startup/

I'm fine with either patch. My issue with D44243 is that the reason for the change wasn't provided. The explanation you provide was satisfactory once I read https://github.com/google/sanitizers/issues/194 .
Without reading the github issue it's not so clear what _asan_init-safe means. I'll leave it up to you to decide whether you want the commit message to be clear when read on its own or whether
referencing the GitHub issue is sufficient.

alekseyshl edited the summary of this revision. (Show Details)Jun 14 2018, 11:15 AM

Isn't this basically https://reviews.llvm.org/D44243 ?

It is! Thanks, missed that one. I'm fine with that one being committed too. Is the explanation on this patch satisfactory?

minor nit: s/on the startup/on application startup/ or s/on the startup/on startup/

I'm fine with either patch. My issue with D44243 is that the reason for the change wasn't provided. The explanation you provide was satisfactory once I read https://github.com/google/sanitizers/issues/194 .
Without reading the github issue it's not so clear what _asan_init-safe means. I'll leave it up to you to decide whether you want the commit message to be clear when read on its own or whether
referencing the GitHub issue is sufficient.

Thank you for the suggestions, updated the message to provide a bit more inline context.

morehouse accepted this revision.Jun 14 2018, 11:20 AM
This revision is now accepted and ready to land.Jun 14 2018, 11:20 AM
This revision was automatically updated to reflect the committed changes.
filcab added a subscriber: filcab.Jun 25 2018, 9:27 AM

This got committed, but clang still warns:

/Users/filcab/dev/llvm/compiler-rt/lib/asan/asan_report.cc:209:39: warning: declaration requires a global constructor [-Wglobal-constructors]
ErrorDescription ScopedInErrorReport::current_error_(LINKER_INITIALIZED);
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~