Skip to content

Commit

Permalink
Fix a bug in absl::SetVLogLevel where a less generic pattern incorr…
Browse files Browse the repository at this point in the history
…ectly removed a more generic one.

### Example:

The user passes `--vmodule=*pipeline*=10` to enable logs in files containing `"pipeline"` in their names:
```
pipeline_a.cc
pipeline_b.cc
pipeline_c.cc
```
Additionally, `pipeline_a.cc` executes `absl::SetVlogLevel("pipeline_a", 10)`.

### Expected behavior:

VLOG level `10` is enabled in all files containing `"pipeline"` in their names. `absl::SetVlogLevel("pipeline_a", 10)` should not affect this, as `pipeline_a.cc` is already covered by `--vmodule=*pipeline*=10` and have the same level there.

### Actual behavior (before the CL):

VLOG level `10` is enabled only in `pipeline_a.cc`. `--vmodule=*pipeline*=10` is ignored.

### Reason:

`absl::SetVlogLevel` is based on `PrependVModuleLocked`. The issue lies in the following code within `PrependVModuleLocked`:

```cc
  // This is a memory optimization to avoid storing patterns that will never
  // match due to exit early semantics. Primarily optimized for our own unit
  // tests.
  get_vmodule_info().erase(
      std::remove_if(++iter, get_vmodule_info().end(),
                     [module_pattern](const VModuleInfo& info) {
                       return FNMatch(info.module_pattern, module_pattern);
                     }),
      get_vmodule_info().cend());
```

When `absl::SetVlogLevel("pipeline_a", 10)` is executed, `get_vmodule_info()` contains `"*pipeline*"`. Therefore, `info.module_pattern` equals to `"*pipeline*"` and `module_pattern` equals to `"pipeline_a"`.

Because `"pipeline_a"` matches the pattern `"*pipeline*"`, the pattern `"*pipeline*"` is erroneously erased. `get_vmodule_info()` then only contains the `"pipeline_a"` pattern.

### Solution:

This CL corrects the order of the arguments in `FNMatch` to:
```cc
FNMatch(module_pattern, info.module_pattern);
```

This ensures that it correctly checks if the new pattern is more general than the previous pattern, but not vice versa.  In the example above, the new pattern `"pipeline_a"` does not match the previous `"*pipeline*"` pattern. Therefore, `get_vmodule_info()` retains both patterns in the following order: `"pipeline_a"`, `"*pipeline*"`.

PiperOrigin-RevId: 675776298
Change-Id: I9550dbd4282e66799619369894b8304856a7a777
  • Loading branch information
Alex Titov authored and copybara-github committed Sep 18, 2024
1 parent 3aa6061 commit 0df5674
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 1 deletion.
9 changes: 9 additions & 0 deletions absl/log/globals_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,15 @@ TEST(TestGlobals, SetVLogLevel) {
EXPECT_EQ(absl::SetVLogLevel("setvloglevel", 42), 0);
EXPECT_EQ(absl::SetVLogLevel("setvloglevel", 1337), 42);
EXPECT_EQ(absl::SetVLogLevel("othersetvloglevel", 50), 0);

EXPECT_EQ(absl::SetVLogLevel("*pattern*", 1), 0);
EXPECT_EQ(absl::SetVLogLevel("*less_generic_pattern*", 2), 1);
// "pattern_match" matches the pattern "*pattern*". Therefore, the previous
// level must be 1.
EXPECT_EQ(absl::SetVLogLevel("pattern_match", 3), 1);
// "less_generic_pattern_match" matches the pattern "*pattern*". Therefore,
// the previous level must be 2.
EXPECT_EQ(absl::SetVLogLevel("less_generic_pattern_match", 4), 2);
}

TEST(TestGlobals, AndroidLogTag) {
Expand Down
9 changes: 8 additions & 1 deletion absl/log/internal/vlog_config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,14 @@ int PrependVModuleLocked(absl::string_view module_pattern, int log_level)
get_vmodule_info().erase(
std::remove_if(++iter, get_vmodule_info().end(),
[module_pattern](const VModuleInfo& info) {
return FNMatch(info.module_pattern, module_pattern);
// Remove the previous pattern if it is less generic than
// the new one. For example, if the new pattern
// `module_pattern` is "foo*" and the previous pattern
// `info.module_pattern` is "foo", we should remove the
// previous pattern. Because the new pattern "foo*" will
// match all the files that the previous pattern "foo"
// matches.
return FNMatch(module_pattern, info.module_pattern);
}),
get_vmodule_info().cend());
return old_log_level.value_or(global_v);
Expand Down
41 changes: 41 additions & 0 deletions absl/log/vlog_is_on_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,47 @@ TEST_F(VLogIsOnTest, PatternWorksWithoutMaxVerbosityAndMinLogLevel) {
VLOG(4) << "spam";
}

TEST_F(VLogIsOnTest,
PatternOverridesLessGenericOneWithoutMaxVerbosityAndMinLogLevel) {
if (MaxLogVerbosity().has_value() || MinLogLevel().has_value()) {
GTEST_SKIP();
}

// This should disable logging in this file
absl::SetVLogLevel("vlog_is_on*", -1);
// This overrides the previous setting, because "vlog*" is more generic than
// "vlog_is_on*". This should enable VLOG level 3 in this file.
absl::SetVLogLevel("vlog*", 3);
absl::ScopedMockLog log(absl::MockLogDefault::kDisallowUnexpected);

EXPECT_CALL(log, Log(absl::LogSeverity::kInfo, _, "important"));

log.StartCapturingLogs();
VLOG(3) << "important";
VLOG(4) << "spam";
}

TEST_F(VLogIsOnTest,
PatternDoesNotOverridesMoreGenericOneWithoutMaxVerbosityAndMinLogLevel) {
if (MaxLogVerbosity().has_value() || MinLogLevel().has_value()) {
GTEST_SKIP();
}

// This should enable VLOG level 3 in this file.
absl::SetVLogLevel("vlog*", 3);
// This should not change the VLOG level in this file. The pattern does not
// match this file and it is less generic than the previous patter "vlog*".
// Therefore, it does not disable VLOG level 3 in this file.
absl::SetVLogLevel("vlog_is_on_some_other_test*", -1);
absl::ScopedMockLog log(absl::MockLogDefault::kDisallowUnexpected);

EXPECT_CALL(log, Log(absl::LogSeverity::kInfo, _, "important"));

log.StartCapturingLogs();
VLOG(3) << "important";
VLOG(5) << "spam";
}

TEST_F(VLogIsOnTest, GlobalDoesNotFilterBelowMaxVerbosity) {
if (!MaxLogVerbosity().has_value() || *MaxLogVerbosity() < 2) {
GTEST_SKIP();
Expand Down

0 comments on commit 0df5674

Please sign in to comment.