[nsan] Make NSAN_OPTIONS consistent

[alexander-shaposhnikov]

https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/nsan/nsan_flags.inc Currently we have log2_max_relative_error log2_absolute_error_threshold. See https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/nsan/nsan.cpp#L468 and https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/nsan/nsan.cpp#L464 .

It would be better to make them true log2, not "-log2" (see https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/nsan/nsan_flags.inc#L32) and support both positive and negative values.

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
3. Fix the issue locally.
4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
5. Create a Git commit.
6. Run git clang-format HEAD~1 to format your changes.
7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

Author: Alexander Shaposhnikov (alexander-shaposhnikov)

https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/nsan/nsan_flags.inc Currently we have `log2_max_relative_error` `log2_absolute_error_threshold`. It would be better to make naming consistent and also fix the semantics of these options (the names are actually misleading). The proposed new names: `log2_max_relative_error` `log2_max_absolute_error`

[aditya-167]

Hello, I am new to LLVM/MLIR, I would love to work on this.

[alexander-shaposhnikov]

This should be relatively straightforward: need to fix the sign of log2 & update the code to support both positive and negative values (this might need some care) Happy to try to answer any questions or provide more code pointers.

[alexander-shaposhnikov]

UPD. After taking a closer look at how the check is organized, I think the names are okay. There are a few aspects of the current behavior that can be improved though. I've updated the description.

[Rajveer100]

@alexander-shaposhnikov If I am not wrong, we want a smallest variable to handle lower bound as well, is that what you meant by to support negative values?

Or we could take the reciprocal of it, ex: (1 / (2 ^ 19)) -> (1 / (2 ^ -19))

[alexander-shaposhnikov]

@Rajveer100 - For testing purposes, I wanted to be able to use both positive and negative values for log2_absolute_error_threshold. However, currently, both options expect nonnegative numbers, and the actual thresholds are (1/2)^n, not 2^n. Changing log2_absolute_error_threshold alone would create inconsistency.

In general, we can leave everything as is, because (I guess this is what you are referring to) the check uses largest https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/nsan/nsan.cpp#L467C20-L467C27 => the relative error <= 1. Perhaps we can keep this issue open in case anyone has any opinions or suggestions..