- User Since
- Jan 31 2017, 4:46 PM (259 w, 17 h)
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.
Fri, Jan 14
Tue, Jan 11
Mon, Jan 10
Ping. This is very similar to the other change I landed recently. It needs approval from a libc++ maintainer.
Fri, Jan 7
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.
Thu, Jan 6
Tue, Jan 4
Sun, Jan 2
Tue, Dec 28
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.
Dec 3 2021
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.
Nov 30 2021
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.
Nov 29 2021
Nov 22 2021
second copy of InstrProfData.inc
Use a DenseMap rather than instruction name to match bias load.
Nov 21 2021
Nov 16 2021
Nov 10 2021
Reordered the switches.
Nov 3 2021
Sep 23 2021
Aug 27 2021
add more casts
Aug 26 2021
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.
Aug 25 2021
Aug 23 2021
Aug 12 2021
Aug 6 2021
Aug 5 2021
Jul 27 2021
Jul 23 2021
Jul 20 2021
Jul 19 2021
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.
Jul 15 2021
Jul 7 2021
Jul 5 2021
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.
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.
Factoring out the arithmetic from the OS-specific code and landing only the Fuchsia version of the OS-specific implementation LGTM.
Jul 1 2021
lgtm modulo some comment cleanups
Jun 30 2021
It makes sense that the contract be that the runtime must define the variable.
Jun 22 2021
Since those only affects sanitizers and not other compiler-rt libraries, perhaps it should be called SANITIZER_* instead of COMPILER_RT_*?
Jun 16 2021
Jun 15 2021
Jun 14 2021
Jun 11 2021
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.
Jun 8 2021
Jun 7 2021
Jun 4 2021
Jun 3 2021
Jun 2 2021
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.
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.