minifi-cpp - Consensus regarding enablement of fsanitize=address

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

minifi-cpp - Consensus regarding enablement of fsanitize=address

Andy Christianson-2
MiNiFi - C++ Devs:


On an earlier commit, I added the following to the root CMakeLists.txt:


# Enable asan in DEBUG for compatibility with civet
set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -fsanitize=address")


This was, as stated in the comment, to address a compatibility issue when compiling in DEBUG where civet would fail to link.


On more recent dev toolchains (devtoolset-6 SCL CentOS/GCC 6.3), this results in extremely verbose and unhelpful output during test runs. While I am not opposed to using these tools to detect legitimate memory leaks, as it stands currently the setting is more of a detriment to the development process. I think that we should either only turn this on when we deliberately intend to analyze memory usage, or possibly create additional unit tests to detect memory leaks.


In the meantime, I think we should remove the statement from our CMakeLists.txt and disable it in the CMakeLists.txt for civet as well.


I would like to hear additional feedback from the development community to see if we have consensus on this change.


Regards,


Andy I.C.
Reply | Threaded
Open this post in threaded view
|

Re: minifi-cpp - Consensus regarding enablement of fsanitize=address

Marc Parisi
Andy,
  I agree with your suggestion. There's no reason why we couldn't make this
an option in the future after its removal to help clean up DEBUG.

  Thanks,
  Marc

On Tue, Oct 24, 2017 at 9:48 AM, Andy Christianson <
[hidden email]> wrote:

> MiNiFi - C++ Devs:
>
>
> On an earlier commit, I added the following to the root CMakeLists.txt:
>
>
> # Enable asan in DEBUG for compatibility with civet
> set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -fsanitize=address")
>
>
> This was, as stated in the comment, to address a compatibility issue when
> compiling in DEBUG where civet would fail to link.
>
>
> On more recent dev toolchains (devtoolset-6 SCL CentOS/GCC 6.3), this
> results in extremely verbose and unhelpful output during test runs. While I
> am not opposed to using these tools to detect legitimate memory leaks, as
> it stands currently the setting is more of a detriment to the development
> process. I think that we should either only turn this on when we
> deliberately intend to analyze memory usage, or possibly create additional
> unit tests to detect memory leaks.
>
>
> In the meantime, I think we should remove the statement from our
> CMakeLists.txt and disable it in the CMakeLists.txt for civet as well.
>
>
> I would like to hear additional feedback from the development community to
> see if we have consensus on this change.
>
>
> Regards,
>
>
> Andy I.C.
>
Reply | Threaded
Open this post in threaded view
|

Re: minifi-cpp - Consensus regarding enablement of fsanitize=address

Andy Christianson-2
Sounds good. I have a local commit which I'll move to a branch and submit a PR for, after adding a relevant JIRA.

Regards,

Andy I.C.
________________________________________
From: Marc <[hidden email]>
Sent: Wednesday, October 25, 2017 8:08 AM
To: [hidden email]
Subject: Re: minifi-cpp - Consensus regarding enablement of fsanitize=address

Andy,
  I agree with your suggestion. There's no reason why we couldn't make this
an option in the future after its removal to help clean up DEBUG.

  Thanks,
  Marc

On Tue, Oct 24, 2017 at 9:48 AM, Andy Christianson <
[hidden email]> wrote:

> MiNiFi - C++ Devs:
>
>
> On an earlier commit, I added the following to the root CMakeLists.txt:
>
>
> # Enable asan in DEBUG for compatibility with civet
> set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -fsanitize=address")
>
>
> This was, as stated in the comment, to address a compatibility issue when
> compiling in DEBUG where civet would fail to link.
>
>
> On more recent dev toolchains (devtoolset-6 SCL CentOS/GCC 6.3), this
> results in extremely verbose and unhelpful output during test runs. While I
> am not opposed to using these tools to detect legitimate memory leaks, as
> it stands currently the setting is more of a detriment to the development
> process. I think that we should either only turn this on when we
> deliberately intend to analyze memory usage, or possibly create additional
> unit tests to detect memory leaks.
>
>
> In the meantime, I think we should remove the statement from our
> CMakeLists.txt and disable it in the CMakeLists.txt for civet as well.
>
>
> I would like to hear additional feedback from the development community to
> see if we have consensus on this change.
>
>
> Regards,
>
>
> Andy I.C.
>


Reply | Threaded
Open this post in threaded view
|

Re: minifi-cpp - Consensus regarding enablement of fsanitize=address

Andy Christianson-2
PR is in [1].

Regards,

Andy I.C.

[1]: https://github.com/apache/nifi-minifi-cpp/pull/154
________________________________________
From: Andy Christianson <[hidden email]>
Sent: Wednesday, October 25, 2017 10:47 AM
To: [hidden email]
Subject: Re: minifi-cpp - Consensus regarding enablement of fsanitize=address

Sounds good. I have a local commit which I'll move to a branch and submit a PR for, after adding a relevant JIRA.

Regards,

Andy I.C.
________________________________________
From: Marc <[hidden email]>
Sent: Wednesday, October 25, 2017 8:08 AM
To: [hidden email]
Subject: Re: minifi-cpp - Consensus regarding enablement of fsanitize=address

Andy,
  I agree with your suggestion. There's no reason why we couldn't make this
an option in the future after its removal to help clean up DEBUG.

  Thanks,
  Marc

On Tue, Oct 24, 2017 at 9:48 AM, Andy Christianson <
[hidden email]> wrote:

> MiNiFi - C++ Devs:
>
>
> On an earlier commit, I added the following to the root CMakeLists.txt:
>
>
> # Enable asan in DEBUG for compatibility with civet
> set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -fsanitize=address")
>
>
> This was, as stated in the comment, to address a compatibility issue when
> compiling in DEBUG where civet would fail to link.
>
>
> On more recent dev toolchains (devtoolset-6 SCL CentOS/GCC 6.3), this
> results in extremely verbose and unhelpful output during test runs. While I
> am not opposed to using these tools to detect legitimate memory leaks, as
> it stands currently the setting is more of a detriment to the development
> process. I think that we should either only turn this on when we
> deliberately intend to analyze memory usage, or possibly create additional
> unit tests to detect memory leaks.
>
>
> In the meantime, I think we should remove the statement from our
> CMakeLists.txt and disable it in the CMakeLists.txt for civet as well.
>
>
> I would like to hear additional feedback from the development community to
> see if we have consensus on this change.
>
>
> Regards,
>
>
> Andy I.C.
>