Please understand that InstrProfData.inc is now a public installed header file in the compiler-rt installation, at <profile/InstrProfData.inc>. So *any* changes to this file have a quite high standard of compatibility concern.

lgtm

Ping. This is very similar to the other change I landed recently. It needs approval from a libc++ maintainer.
Thanks.

LGTM modulo the fallback behaviors as mentioned below. But I'll leave the approval to those who have done the review for this specific codebase more generally, since I'm not sure on all the points of LLVM style or what library features are available or best to use.

Just the fact that any follow-up changes were required is clear testament to the fact that this was never a change that was appropriate to commit without community review and approval as all LLVM changes require. Regardless of the technical details, nontrivial changes, especially ones affecting the user-visible semantics, simply should not be committed without proper review. Resistance to reverting unreviewed changes is simply hostile to the community's established standards and code of conduct and frankly does not merit further technical discussion. The appropriate forum for technical discussion is in review of changes such as these *before* they are committed. The appropriate action right now is to revert all the changes that were not properly reviewed and changed behavior in ways that reviewers have not yet had the opportunity to discuss. Once the damage done has been recovered, then it's appropriate to propose the changes you wanted made and allow proper review and discussion about the semantics being changed.

This feature seems like a good addition. I'm wondering about the interactions with other switches and if it might make sense to rearchitect some of the CLI API here for the more arcane corners. Maybe it's already orthogonal enough, but I'm not sure. I'm thinking in particular about the various --strip-* options. Perhaps it's the case that all such variations apply only to text IFS output? I'm also wondering if there might be use cases for multiple different kinds of IFS output (i.e. different sets of stripping / arch options). Come to think of it, we could even have uses for multiple ELF stub outputs from the same IFS input with different switches (e.g. arch). So perhaps it makes sense to go in a direction where each output file is separately described with both its format (IFS vs ELF) and its format options (stripping, arch, etc), and there can be any number of such output files in whatever combination. I'm not sure exactly what a CLI syntax for that would look like. Another possibility that might fit well for build system integration is to accept a complex output specification in JSON either as a command-line switch value or from a file.

lgtm, but we should be sure to advise users that their debugging tools may need build adjustments to ensure they support the compressed formats.

lgtm, but we should be sure to advise users who may need to use -gdwarf-4 explicitly if their debugging tools become unhappy.

renamed callback

In D114383#3160468, @phosek wrote:

An alternative I thought of would be to always use a call and generate a (weak) implementation of __llvm_profile_counter_dynamic_bias inside each translation unit. The default generated function would simply return the variable. I'm not sure what the impact on performance would be though.

second copy of InstrProfData.inc

Use a DenseMap rather than instruction name to match bias load.

Reordered the switches.

reordered switches

rebased

add more casts

The intent is to sanity-check for garbage header fields. The uncompressed size field is one that you can only warn about heuristically like this I guess. But you can do a definitive test on the CompressedSize field to ensure that it isn't larger than the actual size of the data available. There's no need to even worry about whether UncompressedSize is valid if CompressedSize is clearly invalid.

lgtm

lgtm

lgtm

lgtm

lgtm

lgtm

For Fuchsia you should just define the new macro to continue to use zxtest's ASSERT_DEATH macro.

lgtm modulo typo

I think it would be more in keeping with local style to make this a third #elif SANITIZER_FUCHSIA leg in the middle with the InitShadowGOT stub. FindDynamicShadowStart is only called from Linux-specific code and does not need to be defined.

lgtm with fxbug.dev/NNN url ref added.

lgtm

lgtm

MaskRay's code generation example shows the optimal code possible for implementing this feature and how that code is generated is unaffected by this change, so I don't understand why that subject is being raised here.

lgtm with minor changes + clang-format.

lgtm

Frankly I don't think it makes sense to have __sanitizer_mallinfo and those other allocator functions declared in hwasan_interface_internal.h at all.
Those are neither APIs that the compiler emits nor ones that anyone else should use. They are just implementation details of the allocator interceptors.
I think a better cleanup is to move those declarations out of that file. I don't see why they need to be in any header file rather than just in hwasan_allocation_functions.cpp.

The stated goal is very sensible. But this change is doing too many things at once to get there. Please separate the refactorings into smaller changes that come with some discussion of why that refactoring is needed.

rebased

Factoring out the arithmetic from the OS-specific code and landing only the Fuchsia version of the OS-specific implementation LGTM.

lgtm modulo some comment cleanups

It makes sense that the contract be that the runtime must define the variable.

lgtm

Since those only affects sanitizers and not other compiler-rt libraries, perhaps it should be called SANITIZER_* instead of COMPILER_RT_*?

Usually we like to split changes up into separate small changes for the pure refactoring, and then changes that purely add new Fuchsia-specific code.
I'll do an initial review round of the Fuchsia code here since you've sent it. But then I think this review should be tightened to just the pure refactoring, and we should have a separate review thread for the Fuchsia parts.

lgtm

In D103303#2798557, @peter.smith wrote:

Thanks for the updates.

One idea that might work better for the linker script fragment is to use a new keyword. For example instead of using --overwrite-section-script and SECTIONS. Use a new keyword like OVERWRITE_SECTIONS this could essentially be mixed in with any other linker script without needing --overwrite-section-script.

In D103564#2797657, @leonardchan wrote:

I don't think we can move the __sanitizer_* allocation functions since they're used for aliases for their libc equivalents which require definitions in the same TU:

In D103303#2797099, @MaskRay wrote:

This would add some complexity to readLinkerScript as we will need to check the first token. Do we really need it?
This is just a matter between $file and -Wl,--overwrite-script=$file in the build system.

I think probably the better refactoring here would be to split this file in two: one containing just the allocation functions; and a second with everything else.
On Fuchsia, we'll use the allocation functions but none of the rest of it. So the second whole file could just be under #if !SANITIZER_FUCHSIA.

lgtm

I think this may need to be about the last thing to actually land. It shouldn't land before building the runtime reliably works without error, and I think we should land any refactoring of the runtime we needed in preparation for Fuchsia-specific changes before landing anything Fuchsia-specific to make it compile.