- User Since
- Jan 31 2017, 4:46 PM (155 w, 2 d)
I think I've done everything Vitaly's last review asked for. Thanks.
Split out ThreadContext refactoring into a separate patch.
Updated related changes following review comments.
Sat, Jan 18
I've split part of the patch out and done a substantial refactoring of the standalone lsan thread stuff following Vitaly's review.
Generic refactoring patch split out.
Standalone LSan port refactored following review comments.
Rebased, added a comment.
Thu, Jan 16
Dec 17 2019
Oct 16 2019
It's time to land this now. Can one of you commit it for me?
Aug 24 2019
This should land only after we've done our first stages of downstream roll-out and burn-in testing. But setting it up now.
Aug 15 2019
Aug 14 2019
Aug 4 2019
Jul 17 2019
Jul 11 2019
Jul 3 2019
AFAICT the inlining is the whole point of FastPoisonShadow so this LGTM as is.
Jun 29 2019
Jun 24 2019
Move logic out of RawCoverage constructor.
Jun 23 2019
https://fuchsia-review.googlesource.com/c/fuchsia/+/295508 has the runtime implementation that motivated this.
May 31 2019
May 30 2019
May 28 2019
May 27 2019
May 24 2019
lgtm contingent on verifying intended behavior changes and adding comments. The factoring suggestion for the triple,triple searching is preferred but at your discretion, though I'd really like to see at least comments.
May 21 2019
lgtm with one nit: either change to an assert or improve the comment to clarify what other cases are expected
May 14 2019
May 6 2019
Previously the same local symbol name was being used for both things in an uncoordinated fashion. The logic in places like ValueSymbolTable::createValueName leads to the second one getting renamed with some ".digits" suffix to make them unique.
Then the code that emits the references between the init_array and the actual function and sets the COMDAT group name assumed it could use the original name throughout, but in some places this was replaced with the suffixed name and in some places it was not, leading to the incorrect references.
Sorting R_*_RELATIVE relocs first and setting REL(A)COUNT is a format requirement, period. It's not a debatable question of optimization.
May 5 2019
May 4 2019
This works for my case that led to me to file https://bugs.llvm.org/show_bug.cgi?id=41693 as a linker issue.
May 3 2019
The sorting behavior is entirely an optimization for locality at dynamic reloc time. Both locality of symtab indices and locality of reloc offsets are useful to that goal.
Apr 30 2019
Apr 24 2019
Apr 3 2019
IMHO the library code should use #if !__has_feature(...) to avoid the definitions entirely when built with a sanitizer whose runtime provides them.
But this is a fine way to achieve that while we wait for those libraries to be fixed for sanitized builds.
Apr 2 2019
Mar 28 2019
Feb 16 2019
This is only on GCC trunk (i.e. 9), not in 8.2 or even the current gcc-8 branch. So clarify the log entry. We don't know if 8.x,x>2 will add it or only gcc 9.
Feb 14 2019
Feb 13 2019
I only really looked at the Fuchsia code.
Feb 9 2019
lgtm with a comment (and perhaps an assertion).
Jan 30 2019
Jan 25 2019
Should you add -nostdinc++ to the compilation flags to drive home that this code can't depend on any library headers?
Jan 20 2019
Jan 14 2019
Jan 5 2019
Jan 4 2019
lgtm. Wouldn't hurt to comment that this depends on a linker new enough to support the new reloc types (only reason it's conditional at all) and since we default to lld we don't worry about host system linkers that might be too old to support the new reloc types.
Dec 10 2018
Nov 15 2018
lgtm with a comment in the code
Nov 13 2018
Nov 10 2018
Nov 6 2018
Nov 5 2018
This breaks the semantics we want. The -Bdynamic is there to apply to -lm, which is also what --as-needed is there for in this case (it appears earlier because of the conditional logic, since in the other case it applies to -lc++ as well). It's correct as is.
Nov 3 2018
Nov 2 2018
So the default is not per-target? It should be.
Nov 1 2018
Oct 31 2018
Oct 30 2018
Oct 28 2018
I don't know what's affected by the presence of that declaration. Certainly it should be defined.
AFAICT this has nothing to do with solving the problem of the symbol not existing in the library.
That needs CMakeLists.txt:34 changed so it compiles cxa_thread_atexit.cpp for Fuchsia.
We also need to make sure that the cmake check for presence of __cxa_thread_atexit_impl in libc is passing correctly.
Oct 23 2018
Oct 19 2018
Oct 17 2018
Oct 3 2018
The motivating use case always pairs noderef with an address_space attribute, and it's already invalid to convert between pointer types in different address spaces without a cast.
So I don't think we have a strong opinion one way or the other. In the abstract, I'd say noderef feels like a "constraining" qualifier a la const/volatile so that going from unconstrained to constrained implicitly is OK but not vice versa.
Sep 28 2018