User Details
- User Since
- Jan 31 2017, 4:46 PM (207 w, 4 d)
Yesterday
lgtm.
btw, I think fx test --count will do that for you.
Thu, Jan 21
I am also a C++ programmer working on C++ ABI support. I'm glad to be working on a platform that made many intentional design decisions to enable keeping the C++ ABI out of the platform ABI. It makes it vastly easier to do interesting work on C++ ABIs such as what Leo is doing. It also makes it significantly easier to do interesting work on platform ABIs, which I also do.
Wed, Jan 20
The Fuchsia platform ABI includes the SS & SCS ABI. There is no compatibility issue with those. PIC-friendly optimizations are purely an optimization and correctness issue for any PIC/PIE code, and is not an ABI issue.
The only thing where we have an incompatible difference in ABI between our compiler environments is the C++ ABI.
Fri, Jan 15
Many backends generate PIC-friendly jump tables. This is about generating IR initializers that translate to the same kind of backend assembly expressions as those backends use for their jump tables.
Thu, Jan 14
That plan sounds good to me. There is no way to fold #undef into a macro, so you can't avoid a little bit of boilerplate unless you generate the source code.
Wed, Jan 13
Mon, Jan 11
Fri, Jan 8
You've said you want to avoid expressing the C++ ABI as a generic switch orthogonal to target because people can use it when it's not what they actually want or isn't actually useful to attempt. This is true of all switches affecting ABI and they haven't all been rejected on this basis. But fine, consistency is the hobgoblin of small minds. If the concern there is either user confusion or support burden for configurations nobody wants to test, then both of those are easily addressed by having -fc++abi= constrain the settings it allows based on target. For Fuchsia targets, we want to maintain and test both "fuchsia" and "compat" (or "itanium") modes. If for other targets the only setting allowed is the one that's the default, or if using non-default settings generates a scare warning or suchlike, that's fine. That seems to be the natural way to constrain the explicit control of the C++ ABI to the supported cases, while not conflating control of the C++ ABI with the language-independent platform ABI being targeted.
Thu, Jan 7
It's fine to have a target tuple translate to a default C++ ABI.
But the C++ ABI selection is fundamentally not a target flavor thing. It's just a C++ ABI thing.
So using the target tuple as the sole mechanism to determine C++ ABI is fundamentally wrong.
Tue, Jan 5
Dec 16 2020
lgtm
Dec 4 2020
Dec 2 2020
Dec 1 2020
lgtm
I'm troubled in two ways.
- If this test is polluted by global (TLS) state from prior tests, that seems like a non-hermeticity problem for all these tests. Should we maybe be using a test fixture with SetUp/TearDown to wipe the global state between tests?
- It seems like the code under test actually has semantics that when uninitialized, it will sometimes sample (just rarely). That seems like a problem to me. If the initialized state as configured by the environment will be to never do a gwp allocation, then there should never be a window where one is randomly allowed to happen.
Nov 18 2020
lgtm
updated macro
macro nit
Nov 13 2020
style nit
Nov 10 2020
Nov 3 2020
The GCC documentation specifically gives the example of the standard C function qsort as one that does not qualify as __attribute__((leaf)) because it uses a callback function (that presumably might be from the caller's own TU). AIUI the claim the attribute makes is that no normal control flow (i.e. excluding only signal handlers) would call any function in the caller's TU by any means, nor longjmp to any state captured by a setjmp call in the caller's TU, and thus only normal return or normal C++ exception handling return/unwind would reach the caller's TU again after the jump (call) to this entry point. The caller is thus presumed not to share potentially-called functions with the callee by any means, whether direct symbol references or function pointers passed in the call or function pointers previously stored somewhere (e.g. a C++ vtable). The compiler can make whatever assumptions about potential dataflow/side-effect and the like are implied by this exclusion of TU-local code paths from the otherwise unknown call graph of the external call.
Nov 2 2020
lgtm
Oct 30 2020
LGTM with a few minor changes.
Thanks!
Oct 29 2020
lgtm
Oct 27 2020
There is an RFC going out with this prototype as reference. When there is consensus on the RFC, this will get in shape for landing with complete tests and all.
I already approved this as is. I just think it's dead code.
I think NanoTime is actually not reached at all. We can probably remove it.
This is fine. If we wanted to avoid wider use of _zx_utc_reference_get, I think it would also be fine for this one call to use the C11 timespec_get API rather than a pure Zircon API.
Oct 26 2020
This is fine as is but there's really no need to carefully pack it into a uint64_t. It would be fine for Fuchsia's special header to have to meet some less trivial API. The main complexity there would be just figuring out the header arrangement so that Fuchsia's header could #include something to define the types it's supposed to use. But AFAICT it doesn't really need to be in guarded_pool_allocator.h like this. That generic header could just declare static ThreadLocalPackedVariables *getThreadLocals();. Then the platform header can provide the inline defn outside the class scope, and that header only needs to be used in guarded_pool_allocator.cpp itself.
Thanks!
Oct 24 2020
Oct 23 2020
LGTM. Different names and more comments would be preferable.
I don't really understand why the whole "lazy" logic exists. I can't tell what case it would ever be used in as the code stands. Every thread will pass through ASanThread::Init before it would ever just call fake_stack() and hit the "lazy" case. Is it intended that one could set __asan_option_detect_stack_use_after_return to false (either from startup or explicitly clear it), then create a thread, then set the flag explicitly on later?
Thanks for the fix!
LGTM. The section ordering details are conventional but not supposed to be semantically meaningful anyway.
I'm not sure this is the optimal refactor ultimately, but it seems like a safe and clean pure refactor that's at least an incremental cleanup in expressing what the APIs are being used for.
Oct 8 2020
Oct 2 2020
Sep 29 2020
Two possible approaches to make this kind of problem less likely to come up again:
- don't default the template args. might be annoying for this template.
- replace bool with an enum class NamedForPurpose bool : { kThis = false, kThat = true }; in the template argument type
Sep 28 2020
lgtm
Sep 25 2020
fix typo and missing #include
Sep 24 2020
This is the same as the reverted change except it always defines InstallAtExitCheckLeaks in asan_posix.cpp, just with an empty body on systems without lsan support.
Since everyone approves can someone mark it approved?
Sep 23 2020
LGTM but I'll leave approval to one of the maintainers.
Thanks for the fix!
fix comment typo in earlier change
LGTM modulo clang-format. Thanks for the quick fixes!