Lgtm

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.

lgtm

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.

I only really looked at the Fuchsia code.

lgtm with a comment (and perhaps an assertion).

lgtm

Should you add -nostdinc++ to the compilation flags to drive home that this code can't depend on any library headers?

lgtm

lgtm

lgtm

lgtm

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.

lgtm

lgtm

lgtm

lgtm

lgtm with a comment in the code

lgtm

lgtm

In D53796#1288166, @MaskRay wrote:

Is this patch still needed now that Fuchsia uses:

CmdArgs.push_back("--push-state");
CmdArgs.push_back("--as-needed");
if (OnlyLibstdcxxStatic)
  CmdArgs.push_back("-Bstatic");
ToolChain.AddCXXStdlibLibArgs(Args, CmdArgs);
if (OnlyLibstdcxxStatic)
  CmdArgs.push_back("-Bdynamic");
CmdArgs.push_back("-lm");
CmdArgs.push_back("--pop-state");

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.

lgtm

lgtm

So the default is not per-target? It should be.

lgtm

In D53787#1283051, @rsmith wrote:

In D53787#1282995, @mcgrathr wrote:

In D53787#1282975, @rsmith wrote:

Other symbols must have exactly one definition (modulo the permission for duplicate identical definitions for some cases), but these ones have a default definition that is designed to be overridable by a different definition appearing anywhere in the program.

I don't understand this claim. These are symbols like any others at link time. A single definition must be supplied as for any other function.

The program has two choices: either it provides a definition, and that gets used everywhere, or it does not, and the default version (provided by the toolchain) gets used everywhere.

In D53787#1282975, @rsmith wrote:

These symbols really are special. Other symbols are introduced explicitly by a declaration, whereas these are declared implicitly by the compiler.

In D53787#1279899, @rsmith wrote:

Replacing the global new and delete is supposed to be a whole-program operation (you only get one global allocator). Otherwise you couldn't allocate memory in one DSO and deallocate it in another. (And nor could inline functions etc.)

If you really want to do this weird thing, and you understand what you're getting yourself into, I don't see a problem with having a dedicated flag for it, but don't break all existing users of -fvisibility=.

lgtm

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.

lgtm

lgtm

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.

lgtm

lgtm

lgtm as long as we revert when the underlying bugs are resovled

lgtm

The test will need to verify that the emitted relocs are correct too.

lgtm. Note that most sanitizer runtime ports for Fuchsia will be DSO-only, but I think the #ifndef PIC condition will cover that if/when those cases come up.
For always-static runtimes that can't be statically linked into DSOs (i.e. runtime library is not included in -shared links) this should now be right for Fuchsia.
IIRC profile is always-static but meant to be statically linked separately into each DSO, but presumably that means it's built with PIC defined too.

The log message could give concrete examples.

lgtm

I thought the plan was to produce JSON, not YAML.

I think this wants to be a hard error rather than a warning. Though since we use -Werror anyway if others feel strongly contrary I won't object.

lgtm

lgtm

lgtm

Is this still live? Should it be different after all the multiarch stuff?

LGTM with these few substantive nits and then a whole lot more comments (mostly a more thorough overview covering linkage and output model).

Looks great!

Almost there, but some nits.

lgtm

lgtm

lgtm

In response to Jake's comments about GNUStyle:

lgtm

lgtm

lgtm

lgtm

lgtm

lgtm modulo unnecessary #include

I don't have enough context on how the profiling runtime works to properly review this implementation strategy for Fuchsia. Perhaps adequate comments here would tell me enough. Certainly it needs a lot more comments.
But probably we just need to talk through the implementation plan in person first.

lgtm

LGTM assuming the Fuchsia build still works (I don't see any problems in the source off hand).
@jakehehrlich will verify the Fuchsia Build.

@jakehehrlich is working on an open-source tool at https://fuchsia.googlesource.com/tools that handles this output format as specified in https://fuchsia.googlesource.com/zircon/+/master/docs/symbolizer_markup.md

I'd prefer to keep all the markup string constants together. Moving them all to a header would be fine, but I think it should be a separate sanitizer_symbolizer_fuchsia.h so that non-Fuchsia builds aren't using symbolizer_fuchsia.h, which is about OS-specific stuff distinct from the symbolizer markup format. But nothing in this patch actually needs the constant to be visible outside sanitizer_symbolizer_fuchsia.cc, so I'd like to see some explanation of the need for that.

lgtm with a comment

fnu lgtm

I don't know the backend or the register allocator at all but this looks like a thorough parallel of the existing model for -ffixed-x18.

We should revisit the implicit dependency on fdio. For other compiler-rt libraries' Fuchsia ports, we've kept it at the pure vDSO and minimal libc ABIs.

lgtm

lgtm

lgtm but I'm not authoritative.

lgtm