If you want to contribute to Arm Compute Library, be sure to review the following guidelines.
The development is structured in the following way:
Inclusive language guideline
As part of the initiative to use inclusive language, there are certain phrases and words that were removed or replaced by more inclusive ones. Examples include but not limited to:
- master/slave
- black/white
- he/she, him/her, his/hers
- When referring to a person where gender is irrelevant or unknown, kindly use they, them, theirs, or a person’s preferred pronoun.
Please also follow this guideline when committing changes to Compute Library. It is worth mentioning that the term "master" is still used in some comments but only in reference to external code links that Arm has no governance on.
Futhermore, starting from release (22.05), 'master' branch is no longer being used, it has been replaced by 'main'. Please update your clone jobs accordingly.
Coding standards and guidelines
Best practices (as suggested by clang-tidy):
Helps to prevent undefined behaviour and allows to declare variables const if they are not changed after initialisation. See http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-pro-type-member-init.html
const float32x4_t foo = vdupq_n_f32(0.f);
const float32x4_t bar = foo;
const int32x4x2_t i_foo = {{
vconvq_s32_f32(foo),
vconvq_s32_f32(foo)
}};
const int32x4x2_t i_bar = i_foo;
- No C-style casts (in C++ source code)
Only use static_cast, dynamic_cast, and (if required) reinterpret_cast and const_cast. See http://en.cppreference.com/w/cpp/language/explicit_cast for more information when to use which type of cast. C-style casts do not differentiate between the different cast types and thus make it easy to violate type safety. Also, due to the prefix notation it is less clear which part of an expression is going to be casted. See http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-pro-type-cstyle-cast.html
- No implicit casts to bool
Helps to increase readability and might help to catch bugs during refactoring. See http://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-cast.html
extern int *ptr;
if(ptr){}
if(ptr != nullptr) {}
extern int foo;
if(foo) {}
if(foo != 0) {}
- Use nullptr instead of NULL or 0
The nullptr literal is type-checked and is therefore safer to use. See http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-nullptr.html
- No need to explicitly initialise std::string with an empty string
The default constructor of std::string creates an empty string. In general it is therefore not necessary to specify it explicitly. See http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-string-init.html
std::string foo("");
std::string bar = "";
std::string foo;
std::string bar;
- Braces for all control blocks and loops (which have a body)
To increase readability and protect against refactoring errors the body of control block and loops must be wrapped in braces. See http://clang.llvm.org/extra/clang-tidy/checks/readability-braces-around-statements.html
For now loops for which the body is empty do not have to add empty braces. This exception might be revoked in the future. Anyway, situations in which this exception applies should be rare.
Iterator it;
while(it.next());
- Only one declaration per line
Increase readability and thus prevent errors.
int c, *d;
int e = 0;
int *p = nullptr;
- Pass primitive types (and those that are cheap to copy or move) by value
For primitive types it is more efficient to pass them by value instead of by const reference because:
- the data type might be smaller than the "reference type"
- pass by value avoids aliasing and thus allows for better optimisations
- pass by value is likely to avoid one level of indirection (references are often implemented as auto dereferenced pointers)
This advice also applies to non-primitive types that have cheap copy or move operations and the function needs a local copy of the argument anyway.
More information:
void foo(int i, long l, float32x4_t f);
void bar(const float32x4x4_t &f);
void foobar(
const MyLargeCustomTypeClass &
m);
Unions cannot be used to convert values between different types because (in C++) it is undefined behaviour to read from a member other than the last one that has been assigned to. This limits the use of unions to a few corner cases and therefore the general advice is not to use unions. See http://releases.llvm.org/3.8.0/tools/clang/tools/extra/docs/clang-tidy/checks/cppcoreguidelines-pro-type-union-access.html
- Use pre-increment/pre-decrement whenever possible
In contrast to the pre-increment the post-increment has to make a copy of the incremented object. This might not be a problem for primitive types like int but for class like objects that overload the operators, like iterators, it can have a huge impact on the performance. See http://stackoverflow.com/a/9205011
To be consistent across the different cases the general advice is to use the pre-increment operator unless post-increment is explicitly required. The same rules apply for the decrement operator.
for(size_t i = 0; i < 9; i++);
for(size_t i = 0; i < 9; ++i);
The C and C++ standards don't define a uint type. Though some compilers seem to support it by default it would require to include the header sys/types.h. Instead we use the slightly more verbose unsigned int type.
- Don't use unsigned int in function's signature
Unsigned integers are good for representing bitfields and modular arithmetic. The fact that unsigned arithmetic doesn't model the behavior of a simple integer, but is instead defined by the standard to model modular arithmetic (wrapping around on overflow/underflow), means that a significant class of bugs cannot be diagnosed by the compiler. Mixing signedness of integer types is responsible for an equally large class of problems.
- No "Yoda-style" comparisons
As compilers are now able to warn about accidental assignments if it is likely that the intention has been to compare values it is no longer required to place literals on the left-hand side of the comparison operator. Sticking to the natural order increases the readability and thus prevents logical errors (which cannot be spotted by the compiler). In the rare case that the desired result is to assign a value and check it the expression has to be surrounded by parentheses.
if(nullptr == ptr || false == cond)
{
}
if(ptr == nullptr || cond == false)
{
}
if(ptr = nullptr || cond = false)
{
}
if((ptr = nullptr) || (cond = false))
{
}
Rules
- Use spaces for indentation and alignment. No tabs! Indentation should be done with 4 spaces.
- Unix line returns in all the files.
- Pointers and reference symbols attached to the variable name, not the type (i.e. char &foo;, and not char& foo).
- No trailing spaces or tabs at the end of lines.
- No spaces or tabs on empty lines.
- Put { and } on a new line and increase the indentation level for code inside the scope (except for namespaces).
- Single space before and after comparison operators ==, <, >, !=.
- No space around parenthesis.
- No space before, one space after ; (unless it is at the end of a line).
for(int i = 0; i < width * height; ++i)
{
void *d = foo(ptr, i, &addr);
static_cast<uint8_t *>(data)[i] = static_cast<uint8_t *>(d)[0];
}
- Put a comment after #else, #endif, and namespace closing brace indicating the related name
namespace mali
{
#ifdef MALI_DEBUG
...
#else
...
#endif
}
- CamelCase for class names only and lower case words separated with _ (snake_case) for all the functions / methods / variables / arguments / attributes.
class ClassName
{
public:
void my_function();
int my_attribute() const;
private:
int _my_attribute;
};
- Use quotes instead of angular brackets to include local headers. Use angular brackets for system headers.
- Also include the module header first, then local headers, and lastly system headers. All groups should be separated by a blank line and sorted lexicographically within each group.
- Where applicable the C++ version of system headers has to be included, e.g. cstddef instead of stddef.h.
- See http://llvm.org/docs/CodingStandards.html#include-style
#include "MyClass.h"
#include "arm_cv/core/Helpers.h"
#include "arm_cv/core/Types.h"
#include <cstddef>
#include <numeric>
- Only use "auto" when the type can be explicitly deduced from the assignment.
auto a = static_cast<float*>(bar);
auto b = std::make_unique<Image>(foo);
auto c = img.ptr();
auto d = vdup_n_u8(0);
- OpenCL:
- Use __ in front of the memory types qualifiers and kernel: __kernel, __constant, __private, __global, __local.
- Indicate how the global workgroup size / offset / local workgroup size are being calculated.
- Doxygen:
- No '*' in front of argument names
- [in], [out] or [in,out] in front of arguments
- Skip a line between the description and params and between params and @return (If there is a return)
- Align params names and params descriptions (Using spaces), and with a single space between the widest column and the next one.
- Use an upper case at the beginning of the description
void configure(ITensor *
input, ITensor *output, ActivationLayerInfo activation_info);
How to check the rules
astyle (http://astyle.sourceforge.net/) and clang-format (https://clang.llvm.org/docs/ClangFormat.html) can check and help you apply some of these rules.
Library size: best practices and guidelines
Template suggestions
When writing a new patch we should also have in mind the effect it will have in the final library size. We can try some of the following things:
- Place non-dependent template code in a different non-templated class/method
template<typename T>
class Foo
{
public:
enum { v1, v2 };
};
can be converted to:
struct Foo_base
{
enum { v1, v2 };
};
template<typename T>
class Foo : public Foo_base
{
public:
};
- In some cases it's preferable to use runtime switches instead of template parameters
- Sometimes we can rewrite the code without templates and without any (significant) performance loss. Let's say that we've written a function where the only use of the templated argument is used for casting:
template <typename T>
{
...
*(reinterpret_cast<T *>(out.ptr())) = *(reinterpret_cast<const T *>(in.ptr()));
...
}
The above snippet can be transformed to:
{
...
std::memcpy(out.ptr(), in.ptr(), element_size);
...
}
Secure coding practices
General Coding Practices
- Use tested and approved managed code rather than creating new unmanaged code for common tasks.
- Utilize locking to prevent multiple simultaneous requests or use a synchronization mechanism to prevent race conditions.
- Protect shared variables and resources from inappropriate concurrent access.
- Explicitly initialize all your variables and other data stores, either during declaration or just before the first usage.
- In cases where the application must run with elevated privileges, raise privileges as late as possible, and drop them as soon as possible.
- Avoid calculation errors by understanding your programming language's underlying representation and how it interacts with numeric calculation. Pay close attention to byte size discrepancies, precision, signed/unsigned distinctions, truncation, conversion and casting between types, "not-a-number" calculations, and how your language handles numbers that are too large or too small for its underlying representation.
- Restrict users from generating new code or altering existing code.
Secure Coding Best Practices
- Validate input. Validate input from all untrusted data sources. Proper input validation can eliminate the vast majority of software vulnerabilities. Be suspicious of most external data sources, including command line arguments, network interfaces, environmental variables, and user controlled files.
- Heed compiler warnings. Compile code using the default compiler flags that exist in the SConstruct file.
- Use static analysis tools to detect and eliminate additional security flaws.
- Keep it simple. Keep the design as simple and small as possible. Complex designs increase the likelihood that errors will be made in their implementation, configuration, and use. Additionally, the effort required to achieve an appropriate level of assurance increases dramatically as security mechanisms become more complex.
- Default deny. Base access decisions on permission rather than exclusion. This means that, by default, access is denied and the protection scheme identifies conditions under which access is permitted
- Adhere to the principle of least privilege. Every process should execute with the least set of privileges necessary to complete the job. Any elevated permission should only be accessed for the least amount of time required to complete the privileged task. This approach reduces the opportunities an attacker has to execute arbitrary code with elevated privileges.
- Sanitize data sent to other systems. Sanitize all data passed to complex subsystems such as command shells, relational databases, and commercial off-the-shelf (COTS) components. Attackers may be able to invoke unused functionality in these components through the use of various injection attacks. This is not necessarily an input validation problem because the complex subsystem being invoked does not understand the context in which the call is made. Because the calling process understands the context, it is responsible for sanitizing the data before invoking the subsystem.
- Practice defense in depth. Manage risk with multiple defensive strategies, so that if one layer of defense turns out to be inadequate, another layer of defense can prevent a security flaw from becoming an exploitable vulnerability and/or limit the consequences of a successful exploit. For example, combining secure programming techniques with secure runtime environments should reduce the likelihood that vulnerabilities remaining in the code at deployment time can be exploited in the operational environment.
Guidelines for stable API/ABI
The Application Programming Interface (API) and Application Binary Interface (ABI) are the interfaces exposed to users so their programs can interact with the library efficiently and effectively. Even though changing API/ABI in a way that does not give backward compatibility is not necessarily bad if it can improve other users' experience and the library, contributions should be made with the awareness of API/ABI stability. If you'd like to make changes that affects the library's API/ABI, please review and follow the guidelines shown in this section. Also, please note that these guidelines are not exhaustive list but discussing things that might be easily overlooked.
Guidelines for API
- When adding new arguments, consider grouping arguments (including the old ones) into a struct rather than adding arguments with default values. Introducing a new struct might break the API/ABI once, but it will be helpful to keep the stability.
- When new member variables are added, please make sure they are initialized.
- Avoid adding enum elements in the middle.
- When removing arguments, follow the deprecation process described in the following section.
- When changing behavior affecting API contracts, follow the deprecation process described in the following section.
Guidelines for ABI
We recommend to read through this page and double check your contributions to see if they include the changes listed.
Also, for classes that requires strong ABI stability, consider using pImpl idiom.
API deprecation process
In order to deprecate an existing API, these rules should be followed.
- Removal of a deprecated API should wait at least for one official release.
- Deprecation of runtime APIs should strictly follow the aforementioned period, whereas core APIs can have more flexibility as they are mostly used internally rather than user-facing.
- Any API changes (update, addition and deprecation) in all components should be well documented by the contribution itself.
Also, it is recommended to use the following utility macros which is designed to work with both clang and gcc using C++14 and later.
- ARM_COMPUTE_DEPRECATED: Just deprecate the wrapped function
- ARM_COMPUTE_DEPRECATED_REL: Deprecate the wrapped function and also capture the release that was deprecated
- ARM_COMPUTE_DEPRECATED_REL_REPLACE: Deprecate the wrapped function and also capture the release that was deprecated along with a possible replacement candidate
void DoOldThing();
void DoNewThing();
How to submit a patch
To be able to submit a patch to our development repository you need to have a GitHub account. With that, you will be able to sign in to Gerrit where your patch will be reviewed.
Next step is to clone the Compute Library repository:
git clone "ssh://<your-github-id>@review.mlplatform.org:29418/ml/ComputeLibrary"
If you have cloned from GitHub or through HTTP, make sure you add a new git remote using SSH:
git remote add acl-gerrit "ssh://<your-github-id>@review.mlplatform.org:29418/ml/ComputeLibrary"
After that, you will need to upload an SSH key to https://review.mlplatform.org/#/settings/ssh-keys
Then, make sure to install the commit-msg Git hook in order to add a change-ID to the commit message of your patch:
cd "ComputeLibrary" && mkdir -p .git/hooks && curl -Lo `git rev-parse --git-dir`/hooks/commit-msg https://review.mlplatform.org/tools/hooks/commit-msg; chmod +x `git rev-parse --git-dir`/hooks/commit-msg)
When your patch is ready, remember to sign off your contribution by adding a line with your name and e-mail address to every git commit message:
Signed-off-by: John Doe <john.doe@example.org>
You must use your real name, no pseudonyms or anonymous contributions are accepted.
You can add this to your patch with:
git commit -s --amend
You are now ready to submit your patch for review:
git push acl-gerrit HEAD:refs/for/main
Patch acceptance and code review
Once a patch is uploaded for review, there is a pre-commit test that runs on a Jenkins server for continuous integration tests. In order to be merged a patch needs to:
- get a "+1 Verified" from the pre-commit job
- get a "+1 Comments-Addressed", in case of comments from reviewers the committer has to address them all. A comment is considered addressed when the first line of the reply contains the word "Done"
- get a "+2" from a reviewer, that means the patch has the final approval
At the moment, the Jenkins server is not publicly accessible and for security reasons patches submitted by non-allowlisted committers do not trigger the pre-commit tests. For this reason, one of the maintainers has to manually trigger the job.
If the pre-commit test fails, the Jenkins job will post a comment on Gerrit with the details about the failure so that the committer will be able to reproduce the error and fix the issue, if any (sometimes there can be infrastructure issues, a test platform disconnecting for example, where the job needs to be retriggered).
1.8.13 