Re: [dpdk-dev] [PATCH v13 1/4] enable ASan AddressSanitizer

[David Marchand <david.marchand@redhat.com>]

Replying on this patch since there is no cover letter.

This series looks acceptable to me for rc2.

Patch 3 and 4 will be merged first, since they fix issues that would
be hit with ASan enabled.

I have comments mainly on rewording in commitlogs and documentation.

If you are fine with those comments, I'll fix them before pushing this series.



On Wed, Oct 20, 2021 at 9:47 AM <zhihongx.peng@intel.com> wrote:
>
> From: Zhihong Peng <zhihongx.peng@intel.com>
>
> `AddressSanitizer
> <https://github.com/google/sanitizers/wiki/AddressSanitizer>`_ (ASan)
> is a widely-used debugging tool to detect memory access errors.
> It helps to detect issues like use-after-free, various kinds of buffer
> overruns in C/C++ programs, and other similar errors, as well as
> printing out detailed debug information whenever an error is detected.
>
> We can enable ASan by adding below compilation options:
> -Dbuildtype=debug -Db_lundef=false -Db_sanitize=address
> "-Dbuildtype=debug": This is a non-essential option. When this option
> is added, if a memory error occurs, ASan can clearly show where the
> code is wrong.
> "-Db_lundef=false": When use clang to compile DPDK, this option must
> be added.
>
> Signed-off-by: Xueqin Lin <xueqin.lin@intel.com>
> Signed-off-by: Zhihong Peng <zhihongx.peng@intel.com>
> Acked-by: John McNamara <john.mcnamara@intel.com>

This patch affects the build process so it should be reflected in the
patch title.
I find the commitlog hard to read.


Suggesting following rewording:
"""
build: enable AddressSanitizer

AddressSanitizer [1] a.k.a. ASan is a widely-used debugging tool to detect
memory access errors.
It helps to detect issues like use-after-free, various kinds of buffer
overruns in C/C++ programs, and other similar errors, as well as
printing out detailed debug information whenever an error is detected.

ASan is integrated with gcc and clang and can be enabled via a meson
option: -Db_sanitize=address
See the documentation for details (especially regarding clang).

Enabling ASan has an impact on performance since additional checks are
added to generated binaries.

Enabling ASan with Windows is currently not supported in DPDK.


1: https://github.com/google/sanitizers/wiki/AddressSanitizer
"""

> ---
> v7: 1) Split doc and code into two.
>     2) Modify asan.rst doc
> v8: No change.
> v9: 1) Add the check of libasan library.
>     2) Add release notes.
> v10:1) Split doc and code into two.
>     2) Meson supports asan.
> v11:Modify the document.
> v12:No change.
> v13:Modify the document.
> ---
>  config/meson.build                     | 16 ++++++++++++++
>  devtools/words-case.txt                |  1 +
>  doc/guides/prog_guide/asan.rst         | 30 ++++++++++++++++++++++++++
>  doc/guides/prog_guide/index.rst        |  1 +
>  doc/guides/rel_notes/release_21_11.rst |  9 ++++++++
>  5 files changed, 57 insertions(+)
>  create mode 100644 doc/guides/prog_guide/asan.rst
>
> diff --git a/config/meson.build b/config/meson.build
> index 4cdf589e20..f02b0e9c6d 100644
> --- a/config/meson.build
> +++ b/config/meson.build
> @@ -411,6 +411,22 @@ if get_option('b_lto')
>      endif
>  endif
>
> +if get_option('b_sanitize') == 'address' or get_option('b_sanitize') == 'address,undefined'
> +    if is_windows
> +        error('ASan is not supported on windows')
> +    endif

I see clang started supporting ASan for Windows in version 8.
https://releases.llvm.org/8.0.0/tools/clang/docs/AddressSanitizer.html

I also found some blog about adding ASan support in MSVC.
https://devblogs.microsoft.com/cppblog/addresssanitizer-asan-for-windows-with-msvc/

Keeping this limitation is acceptable for now, but I added a mention
in commitlog.


> +
> +    if cc.get_id() == 'gcc'
> +        asan_dep = cc.find_library('asan', required: true)
> +        if (not cc.links('int main(int argc, char *argv[]) { return 0; }',
> +                dependencies: asan_dep))
> +            error('broken dependency, "libasan"')
> +        endif
> +        add_project_link_arguments('-lasan', language: 'c')
> +        dpdk_extra_ldflags += '-lasan'
> +    endif
> +endif
> +
>  if get_option('default_library') == 'both'
>      error( '''
>   Unsupported value "both" for "default_library" option.
> diff --git a/devtools/words-case.txt b/devtools/words-case.txt
> index 0bbad48626..ada6910fa0 100644
> --- a/devtools/words-case.txt
> +++ b/devtools/words-case.txt
> @@ -5,6 +5,7 @@ API
>  Arm
>  armv7
>  armv8
> +ASan
>  BAR
>  CRC
>  DCB
> diff --git a/doc/guides/prog_guide/asan.rst b/doc/guides/prog_guide/asan.rst
> new file mode 100644
> index 0000000000..6888fc9a87
> --- /dev/null
> +++ b/doc/guides/prog_guide/asan.rst
> @@ -0,0 +1,30 @@
> +.. SPDX-License-Identifier: BSD-3-Clause
> +   Copyright(c) 2021 Intel Corporation
> +
> +Running AddressSanitizer
> +========================
> +
> +`AddressSanitizer
> +<https://github.com/google/sanitizers/wiki/AddressSanitizer>`_ (ASan)
> +is a widely-used debugging tool to detect memory access errors.
> +It helps to detect issues like use-after-free, various kinds of buffer
> +overruns in C/C++ programs, and other similar errors, as well as
> +printing out detailed debug information whenever an error is detected.
> +
> +AddressSanitizer is a part of LLVM (3.1+) and GCC (4.8+).
> +
> +Add following meson build commands to enable ASan in the meson build system:
> +
> +* gcc::
> +
> +    -Dbuildtype=debug -Db_sanitize=address
> +
> +* clang::
> +
> +    -Dbuildtype=debug -Db_lundef=false -Db_sanitize=address

Suggests adding some explanations here and replacing like:

"""
...

AddressSanitizer is a part of LLVM (3.1+) and GCC (4.8+).

Enabling ASan is done by passing the -Db_sanitize=address option to
the meson build system,
see :ref:`linux_gsg_compiling_dpdk` for details.

The way ASan is integrated with clang requires to allow undefined
symbols when linking code.
To do this, the -Db_lundef=false option must be added.

Additionally, passing -Dbuildtype=debug option might help getting more
readable ASan reports.

Example::

  - gcc: meson setup -Db_sanitize=address <build_dir>
  - clang: meson setup -Db_sanitize=address -Db_lundef=false <build_dir>
"""

> +
> +.. Note::
> +
> +    a) If compile with gcc in centos, libasan needs to be installed separately.
> +    b) If the program is tested using cmdline, you may need to execute the
> +       "stty echo" command when an error occurs.

"""
  - The libasan package must be installed when compiling with gcc in
Centos/RHEL.
  - If the program is tested using cmdline, you may need to execute the
    "stty echo" command when an error occurs.
"""

> diff --git a/doc/guides/prog_guide/index.rst b/doc/guides/prog_guide/index.rst
> index 89af28dacb..b95c460b19 100644
> --- a/doc/guides/prog_guide/index.rst
> +++ b/doc/guides/prog_guide/index.rst
> @@ -71,4 +71,5 @@ Programmer's Guide
>      writing_efficient_code
>      lto
>      profile_app
> +    asan
>      glossary
> diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
> index 3362c52a73..10f4275b1b 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -173,6 +173,15 @@ New Features
>    * Added tests to verify tunnel header verification in IPsec inbound.
>    * Added tests to verify inner checksum.
>
> +* **Enable ASan AddressSanitizer.**

Should be in past form:

* **Added ASan support.**





> +
> +  `AddressSanitizer
> +  <https://github.com/google/sanitizers/wiki/AddressSanitizer>`_ (ASan)
> +  is a widely-used debugging tool to detect memory access errors.
> +  It helps to detect issues like use-after-free, various kinds of buffer
> +  overruns in C/C++ programs, and other similar errors, as well as
> +  printing out detailed debug information whenever an error is detected.


-- 
David Marchand