Page MenuHomePhabricator

denik (Denis Nikitin)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 2 2019, 2:52 PM (158 w, 4 d)

Recent Activity

Wed, Aug 3

denik added a comment to D131084: Add support for specifying the severity of a SARIF Result..

Submitting for review:

Some notes:

There are a couple of ways I think we can acheive this, per the spec:

  1. The reportingDescriptor (rule) objects can be given a default configuration property, which can set the default warning level and other data such as rule parameters etc.
  2. The reportingDescriptor objects can omit the default configuration (which then allows operating with warning as default), and the level is then set when the result is reported.

The first approach would be "more correct", what are your thoughts on this? Would we benefit from having per-diagnostic configuration?

There is also the question about the "kind" of results in clang. From my reading of the spec, it seems that "fail" is the only case that applies to us because:

  • "pass": Implies no issue was found.
  • "open": This value is used by proof-based tools. It could also mean additional assertions required
  • "informational": The specified rule was evaluated and produced a purely informational result that does not indicate the presence of a problem
  • "notApplicable": The rule specified by ruleId was not evaluated, because it does not apply to the analysis target.

Of these "open" and "notApplicable" seem to be ones that *could* come to use but I'm not sure where / what kind of diagnostics would use these. Probably clang-tidy's bugprone-* suite?

Let me know what you think is a good way to approach this wrt clang's diagnostics system.

Hmm, we can probably use "informational" for notes, warnings, and remarks, but I'm kinda partial to proposing the latter two upstream.

Wed, Aug 3, 12:22 PM · Restricted Project, Restricted Project

Tue, Jul 19

denik accepted D129886: [clang] Add -fdiagnostics-format=sarif option for future SARIF output.
Tue, Jul 19, 11:13 AM · Restricted Project, Restricted Project

Mon, Jul 18

denik added a comment to D129886: [clang] Add -fdiagnostics-format=sarif option for future SARIF output.

Thanks for adding the flag!

Mon, Jul 18, 12:03 PM · Restricted Project, Restricted Project

Jul 7 2022

denik accepted D128372: [Clang-Tidy] Empty Check.

Thanks Abraham!

Jul 7 2022, 2:22 PM · Restricted Project, Restricted Project

Jun 27 2022

denik added a comment to D128372: [Clang-Tidy] Empty Check.

Thanks for adding a check! Please check my comments.

Jun 27 2022, 12:13 PM · Restricted Project, Restricted Project

Feb 11 2022

denik added a comment to D109794: [Sanitizer] Allow setting the report path to create directory.

Are you able to patch in D119495 and see if that fixes the issue?

Yes. D119495 fixed the problem. Thanks for the quick solution!

Great, will try to get this submitted today, once I address the comments.

Just curious, did __llvm_profile_recursive_mkdir also create directories in a similar way (ignoring EEXIST status)? I'm asking because the same test was creating a PGO profile in a sandbox and it did not fail.

Interesting - I actually modeled the implementation here exactly on that function. It has the same unconditional mkdir with the same comment about ignoring failures due to existing directories.

While debugging the sandbox (before your fix), I noticed that the test crashed when the sandbox was checking lstat of the path from __sanitizer::CreateDir(). Can it be that the sanitizer intercepted the call but since it was __asan::AsanInitInternal() the handler was not set up yet? This would explain why it worked for PGO.

Not totally following - which handler?

Feb 11 2022, 4:05 PM · Restricted Project

Feb 10 2022

denik added a comment to D109794: [Sanitizer] Allow setting the report path to create directory.

This change breaks tests with sanitizers running in a sandbox.
If /usr/local/tmp/asan is in the allow-list for writing, the sandbox will kill the test when it tries to create /usr.

This failure is caught on Chrome OS, https://issuetracker.google.com/209296420.

It sounds like the issue is showing up with some Chrome OS tests built with -fsanitize=address, as opposed to in the unit test added here - is that correct?

Correct, I meant Chrome OS tests. Chrome OS uses Gentoo sandboxing for its unit tests.

If the former, by default the report_file is stderr - do you know where/how the report file path is being set to something else? Note the change only creates a directory for the given report_file path, the sanitizer change itself doesn't change the path. Before the report file creation would fail if the directory didn't exist.

log_path is set in https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/profiles/base/profile.bashrc#512
Note that the directory was created in the same function. And it's called before entering a sandbox.

Can we skip CreateDir in RecursiveCreateParentDirs if the path exists? So it will be no-op in our case?

Oh I see, we normally assume it will error (but we ignore the error) if the directory already exists. But in the sandboxed case it doesn't like the creation attempt, even though the dir actually exists.

Yeah, we can presumably use "stat" to check before trying to create. Actually, there is already a FileExists() in sanitizer_file.h that has implementations in the various OS specific sanitizer files. Most (except windows) call stat (but look for a regular file). I can just add something like a DirExists() interface that has the equivalent dir check using the same interfaces in each OS implementation. Will work on that tomorrow.

Are you able to patch in D119495 and see if that fixes the issue?

Feb 10 2022, 10:18 PM · Restricted Project

Feb 9 2022

denik added a comment to D109794: [Sanitizer] Allow setting the report path to create directory.

This change breaks tests with sanitizers running in a sandbox.
If /usr/local/tmp/asan is in the allow-list for writing, the sandbox will kill the test when it tries to create /usr.

This failure is caught on Chrome OS, https://issuetracker.google.com/209296420.

It sounds like the issue is showing up with some Chrome OS tests built with -fsanitize=address, as opposed to in the unit test added here - is that correct?

Correct, I meant Chrome OS tests. Chrome OS uses Gentoo sandboxing for its unit tests.

Feb 9 2022, 10:13 PM · Restricted Project
denik added a comment to D109794: [Sanitizer] Allow setting the report path to create directory.

This change breaks tests with sanitizers running in a sandbox.
If /usr/local/tmp/asan is in the allow-list for writing, the sandbox will kill the test when it tries to create /usr.

Feb 9 2022, 6:39 PM · Restricted Project

Jan 17 2020

denik added a comment to D68912: Adds -Wrange-loop-analysis to -Wall.

We are hitting the warning in template code with bool and loop like this: "for (const auto& element : value)" where element is bool from template. But not always.
When it is bool the warning triggers:
error: loop variable 'element' is always a copy because the range of type 'const std::vector<bool, allocator<bool> >' does not return a reference [-Werror,-Wrange-loop-analysis]

Jan 17 2020, 12:09 PM · Restricted Project

Sep 12 2019

denik added inline comments to D52524: Add -Wpoison-system-directories warning.
Sep 12 2019, 8:20 AM · Restricted Project
denik updated the diff for D52524: Add -Wpoison-system-directories warning.

Combined two if into one.

Sep 12 2019, 8:19 AM · Restricted Project

Sep 11 2019

denik added a comment to D52524: Add -Wpoison-system-directories warning.

Ping @aaron.ballman , please verify the change.

Sep 11 2019, 11:52 AM · Restricted Project

Aug 28 2019

denik added a comment to D52524: Add -Wpoison-system-directories warning.

Hi Aaron,

Aug 28 2019, 5:54 PM · Restricted Project
denik updated the diff for D52524: Add -Wpoison-system-directories warning.
Aug 28 2019, 5:19 PM · Restricted Project

Aug 19 2019

denik updated the diff for D52524: Add -Wpoison-system-directories warning.

Removed check for libraries.

Aug 19 2019, 4:34 PM · Restricted Project
denik retitled D52524: Add -Wpoison-system-directories warning from Add -Wno-poison-system-directories flag to Add -Wpoison-system-directories warning.
Aug 19 2019, 4:33 PM · Restricted Project
denik added a comment to D52524: Add -Wpoison-system-directories warning.

Manoj, please check updated diff.

Aug 19 2019, 11:33 AM · Restricted Project
denik updated the diff for D52524: Add -Wpoison-system-directories warning.
Aug 19 2019, 11:33 AM · Restricted Project
denik added inline comments to D52524: Add -Wpoison-system-directories warning.
Aug 19 2019, 10:38 AM · Restricted Project
denik updated the diff for D52524: Add -Wpoison-system-directories warning.

Changed Wpoison-system-directories warning to be disabled by default.

Aug 19 2019, 10:37 AM · Restricted Project

Aug 14 2019

denik updated the diff for D52524: Add -Wpoison-system-directories warning.

Fixed clang-format.

Aug 14 2019, 3:41 PM · Restricted Project
denik updated the diff for D52524: Add -Wpoison-system-directories warning.

Updated the code (removed Diag propagation).
Added test cases.

Aug 14 2019, 3:00 PM · Restricted Project

Aug 8 2019

denik commandeered D52524: Add -Wpoison-system-directories warning.

Taking ownership of the change as Yunlian is no longer working on this CL.

Aug 8 2019, 10:02 AM · Restricted Project