If you're interested in contributing to PLCrashReporter, welcome! Please consider perusing the contributor guide, and contacting us on the project mailing list regarding your area of interest; we're happy to provide assistance in getting started, feedback, and early architecture and implementation reviews.
Our first and principal precept in developing PLCrashReporter is to do no harm. A crash reporter sits at a critical intersection between developers and their customers – if PLCrashReporter fails to accurately and reliably report crashes, this has significant deleterious impact on a developer's ability to find and fix bugs, and thus, an indirect negative impact on the customers who experience those bugs. Even worse, if a crash reporter causes crashes itself, we introduce a very negative direct cost to a developer's customers, and ultimately the developer themselves.
We consider it our responsibility to do everything reasonably in our power to ensure the reliability of PLCrashReporter. However, humans are inescapably imperfect, and thus all software has bugs; as such, we've adopted a fairly strict set of development standards that are intended to account for our fallibility:
- Clean, well-structured, well-documented code helps us to avoid writing bugs to begin with.
- Extensive unit and regression testing helps us catch the bugs that do slip through.
- Careful release and deployment processes give us an opportunity to catch bugs in the wild before they are exposed to a wider audience.
- Paid support subscriptions give us deeper insight into the reliability and usage of PLCrashReporter in the wild, by giving us deeper NDA'd access to customer code and binaries that would not be available otherwise, in exchange for our providing assistance with integration issues and deep analysis of crash reports.
Comprehensive unit testing is required. Tests should be written in concert with the code being tested, not afterward as a perfunctory task prior to patch submission. It is when writing the code that you have the fullest understanding of the potential failure cases and bugs that may be in residence, and unit testing ensures that you write the code in such a way that these failure modes can be tested.
Tests that are added later are invariably cursory; it's simply much harder to fully comprehend a complex system to the same degree that you did when writing it, and you've lost any chance for integrating architectural lessons learned while writing testable components into your final patch.
- All non-UI code should be unit tested.
- If it can't be unit tested, figure out how it can be.
- If it still can't be unit tested, write an integration test for it.
- When reviewing code, make sure it's tested.
Well-documented code helps us to avoid writing bugs by making expected invariants clear to both the implementor and later readers.
Write documentation at the same time you write the code, for the exact same reason as we require test-driven development. Programming languages — especially ones with lackluster type systems — are simply incapable of expressing the full set of invariants of a piece of code. The best time to explain those invariants are when you have them in mind, as you're writing the code in question.
Additionally, documenting code will often reveal bugs and design flaws, as it forces you to think through the systemic invariants of the code you're writing, and how it fits into the larger whole.
We currently use doxygen syntax, which is largely compatible with clang's new -Wdocumentation flag and documentation parser. We may switch to another clang-based documentation generator at a future date.
- All code must be documented. Classes, methods, instance variables, functions, parameters, constants. Everything.
- Document code at the time you write it. That's when you'll write the most useful documentation.
- If you need to express something that is more complex than can be expressed in basic API documentation, use doxygen's support for distinct documentation pages, and write up higher-level documentation.
- When reviewing code, make sure it's documented in full.
In implementing data gathering, the crash reporter, by default, must not gather memory state that may contain user data. For our purposes, we define user data as being any in-memory data that was not shipped with the binary itself and was not generated anonymously and generically by the operating system, with the sole exception of the industry-standard collection of register state data.
This requirement helps us to ensure that:
- We do not incur any negative platform vendor response to the use of the library, which could potentially impact all integrators of the library.
- We avoid hindering integrator's compliance with any legal and/or contractual requirements regarding the collection of user data, including:
- The Apple Developer Agreement's proscription against gathering user data.
- National privacy laws.
- Any existing privacy policies maintained by integrators.
In concrete technical terms, this requirement strictly proscribes the collection of data from:
- Any thread's stack.
- The contents of heap memory.
- Data that may be of unknown providence. For example, an entire page of data is gathered, and it is not known that the given page was perviously zero initialized (eg, in the case of local allocator page re-use), there may exist the risk of gathering user data from the non-overwritten portions of the page.
Given the above constraints, all analysis of the process stack is performed at crash time, and the memory contents are never collected by PLCrashReporter.
As a rarity, we may explicitly grant an exception to this rule for features targeted at development-only environments, if:
- The feature is disabled by default, ideally disabled in the build entirely.
- The privacy issues are documented in detail.
- Providing the feature does not risk a negative platform vendor response due to widespread misuse.
No assumptions should be made regarding the async-safety of any API. In cases where the choice is between providing additional functionality, or maintaining guaranteed async-safety, async-safety in the crash handling path is required. Note that this specifically precludes non-async functions such as pthread_mach_thread_np, pthread_getname_np, and dispatch_queue_get_label.
Private API and ABI
No reliance may be made on undocumented private API or ABI.
As a rarity, exceptions may be granted for use of private ABI (not API), subject to the following fail-safe restrictions and guarantees. There exists significant maintenance costs in terms of maintaining portability across platform and operating system releases, and as such, the integration of such features will be rarely, if ever, approved:
- The API/ABI in question is required by constraints of the platform ABI to remain valid for the lifetime of that architecture (thus guaranteeing ongoing correctness).
- The feature enabled by this usage is non-critical; should a new architecture be released under which the given ABI can no longer be used, it must be possible to for the crash reporter to continue to operate, and the feature must not hinder portability.
- In rare cases and after due consideration by the lead developers, non-stable ABI (NOT private API) may be used if:
- The feature is non-critical – PLCrashReporter may operate successfully without it.
- The implementation is fail-safe. That is, if the ABI assumptions are broken, no invalid data will be provided, no crashes will occur, and there will be no negative impact to users of PLCrashReporter other than the loss of the non-critical feature.
- The feature disabled by default
- A feature flag must be provided to allow integrators to ship with the feature excluded from the binary entirely.
- Runtime configuration of the option should be documented with a full with:
- An accounting of potential failure modes (including plain language explaining the worst-case behavior)
- The platform/ABI/ assumptions relied upon for the operation of the feature.
Coding Style and Language Selection
PLCrashReporter makes use of 4-space indentation and BSD-style brackets (brackets on the same line, except for multi-line declarations). In general, simply follow the existing style and you'll be fine.
Any Obejctive-C or C++ classes that support subclassing -- and methods that may be overridden -- must explicitly documented. As a rule, classes should not support subclassing, and composition-based APIs should be used instead. We explicitly disallow subclassing due to the complexity of reasoning through defined invariants once one introduces the possibility of subclasses overriding arbitrary entry points into that system.
PLCrashReporter makes use of C, C++, and Objective-C. The following guidelines apply when selecting a language for a particular implementation:
- All external facing code should be written in the primary platform language. For Mac OS X and iOS, this is Objective-C.
- All internal portable APIs (eg, code that will be shared across platforms) and async-safe code must be written in C++.
Standard Objective-C camel case style naming is used. Classes should be prefixed with 'PL', and should generally follow the convention of [Prefix][Project][Module][Name] -- eg, [PL][Crash][Report][RegisterInfo].
Properties and Instance Variables
Any internal object state should be defined using private instance variables. If state is to be exposed externally, read-only properties should be used; in general, objects should not vend mutable state.
Protected instance variables should never be used.
Neither KVO nor KVC should be used. Any state exposed an object should be formally defined via documented properties, and any observation of those properties should be done using formal delegate/listener protocols.
Functions and types should be all lowercase, using _, not camel case. All C types in the project must be named with a plcrash_prefix. If the type is usable in an async-safe context the prefix must be plcrash_async_. If a function is not async-safe, but operates on async-safe types (ie, code that uses non-async-safe code to initialize state that may be used within the async-safe runtime), a prefix of plcrash_nasync_ must be used.
The below standards are relatively new, and not all code (especially code translated from a previous C implementation) complies. This will be rectified as part of our C++ Migration, and all new code is expected to conform to the below requirements.
In general, C++ code must be written to be readable unambiguously and at a glance by any capable programmer familiar with a reasonable set of languages.
- C++ should only be used where a more platform-native language (eg, such as ObjC) is not viable:
- Internal portable APIs.
- Async-safe code paths.
- Avoid overly complex or opaque constructs wherever possible. The audience for this code is not constrained to C++ specialists, and we want to enable usage and development by a wide range of consumers.
- The C++ standard library may not be used; PLCrashReporter must be usable without linking to the standard library, as to both avoid introducing an unnecessary dependency, reduce the risk of accidental usage of stdlib from async-safe C++ code paths, and to avoid any incompatibilities that may arise due to the mixture of use of libstdc++ and libc++ in consumer applications.
- Exceptions must not be used, as C++ does not support typed exceptions, and thus there exists no compiler-validated mechanism for guaranteeing that all possible exceptions are handled. This introduces unacceptable risk, in that failing to handle an unexpected exception may result in termination, corrupted internal state, or even corrupted non-transient data.
All classes, functions, and types must be declared in the plcrash namespace. If the type is usable in an async-safe context, it must be declared within the plcrash::async namespace. If a function or method is not async-safe, but it exists within the async-safe namespace due to operating on async-safe types (eg, non-async-safe code that initializes state that may be used within the async-safe runtime), a method or function suffix of _nasync must be used.
Classes should be named using camel case, and should generally follow the convention of [Module][Name] -- eg, [Report][RegisterInfo]. Method names should use camel case.
- Headers must use an extension of .hpp
- Source files must use an extension of .cpp
- Objective-C++ source files must use an extension of .mm
- No labels