Page MenuHomePhabricator

[clang][patch] Modify sanitizer options names: renaming blacklist to blocklist
AbandonedPublic

Authored by mibintc on Feb 6 2021, 6:43 AM.

Details

Reviewers
aaron.ballman
echristo
MaskRay
vitalybuka
Group Reviewers
Restricted Project
Summary

This changes the option names that include substring blacklist to blocklist. Also there are some "resource directory" builtin file names. I changed those from blacklist to blocklist. I looked in the doc to see if the builtin file names appear in the documentation, I couldn't find hits using e.g. "asan_blacklist.txt site:llvm.org" so I cautiously think they aren't documented and therefore fair game.

In options.td there is a way to officially mark an option deprecated by adding it to a specific group, I didn't do that yet. When could we actually eliminate the old spelling? How about December 2021?

I thought it would be best to start with the external clang interface, but i also want to make more patches to eliminate whitelist and blacklist in the comments and in the object names, file names etc.

Diff Detail

Unit TestsFailed

TimeTest
90 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::default_blacklist.cpp
Script: -- : 'RUN: at line 6'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -shared-libasan -### /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/asan/TestCases/default_blacklist.cpp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/asan/TestCases/default_blacklist.cpp
80 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::default_blacklist.cpp
Script: -- : 'RUN: at line 6'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -### /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/asan/TestCases/default_blacklist.cpp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/asan/TestCases/default_blacklist.cpp
80 msx64 debian > MemorySanitizer-X86_64.MemorySanitizer-X86_64::default_blacklist.cpp
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=memory -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -gline-tables-only -### /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/msan/default_blacklist.cpp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/msan/default_blacklist.cpp
100 msx64 debian > MemorySanitizer-lld-X86_64.MemorySanitizer-lld-X86_64::default_blacklist.cpp
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=memory -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -fuse-ld=lld -gline-tables-only -### /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/msan/default_blacklist.cpp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/msan/default_blacklist.cpp
40 msx64 debian > cfi-devirt-lld-thinlto-x86_64.cfi-devirt-lld-thinlto-x86_64::anon-namespace.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -m64 -fuse-ld=lld -flto=thin -fsanitize=cfi -fwhole-program-vtables --driver-mode=g++ -fvisibility=hidden -c -DTU1 -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/cfi/Devirt-lld-thinlto-x86_64/Output/anon-namespace.cpp.tmp1.o /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/cfi/anon-namespace.cpp
View Full Test Results (120 Failed)

Event Timeline

mibintc created this revision.Feb 6 2021, 6:43 AM
mibintc requested review of this revision.Feb 6 2021, 6:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2021, 6:43 AM

Thank you for working on this!

This changes the option names that include substring blacklist to blocklist.

I think this change works in some places, but in other places we say things like "blocklisted" which feels a bit awkward. I don't super love the name blocklist, but I don't super hate it either. All the alternative names I come up with aren't really great either, like UnsanitiziedEntities or IgnoredObjects. Maybe someone will have a better idea here than me, but I do think the current name is an improvement over the old name.

In options.td there is a way to officially mark an option deprecated by adding it to a specific group, I didn't do that yet. When could we actually eliminate the old spelling? How about December 2021?

I think we want to leave the deprecated options in for a full release cycle, so I'd say that (assuming this lands in Clang 13) we could remove support once we start work on Clang 14.

I thought it would be best to start with the external clang interface, but i also want to make more patches to eliminate whitelist and blacklist in the comments and in the object names, file names etc.

Fantastic, thank you!

clang/include/clang/AST/ASTContext.h
565 ↗(On Diff #321933)

The comment should be updated as well.

clang/lib/Driver/SanitizerArgs.cpp
141–145

Do we want to retain support for the old file names and warn about using a deprecated name if one is used? This would keep folks working who are relying on this (even if it's undocumented).

Thank you for working on this!

This changes the option names that include substring blacklist to blocklist.

I think this change works in some places, but in other places we say things like "blocklisted" which feels a bit awkward. I don't super love the name blocklist, but I don't super hate it either. All the alternative names I come up with aren't really great either, like UnsanitiziedEntities or IgnoredObjects. Maybe someone will have a better idea here than me, but I do think the current name is an improvement over the old name.

There is a pre-existing patch https://reviews.llvm.org/D82244 using block and allow, that's why I chose blocklist. Although going in cold I would prefer to use allow and deny, and block seems to me a little like cheating (too similar to black). However I can "disagree and commit" (cf. https://en.wikipedia.org/wiki/Disagree_and_commit) with this choice of wording. I can search out the awkward Blocklisted and come up with different phrasing in the prose, but I think names in the program should use blocklist so that they can be easily connected to the option name for future maintainers of llvm. I'll check this.

In options.td there is a way to officially mark an option deprecated by adding it to a specific group, I didn't do that yet. When could we actually eliminate the old spelling? How about December 2021?

I think we want to leave the deprecated options in for a full release cycle, so I'd say that (assuming this lands in Clang 13) we could remove support once we start work on Clang 14.

I thought it would be best to start with the external clang interface, but i also want to make more patches to eliminate whitelist and blacklist in the comments and in the object names, file names etc.

Fantastic, thank you!

Thank you for working on this!

This changes the option names that include substring blacklist to blocklist.

I think this change works in some places, but in other places we say things like "blocklisted" which feels a bit awkward. I don't super love the name blocklist, but I don't super hate it either. All the alternative names I come up with aren't really great either, like UnsanitiziedEntities or IgnoredObjects. Maybe someone will have a better idea here than me, but I do think the current name is an improvement over the old name.

There is a pre-existing patch https://reviews.llvm.org/D82244 using block and allow, that's why I chose blocklist. Although going in cold I would prefer to use allow and deny, and block seems to me a little like cheating (too similar to black).

That was my concern as well -- I can easily imagine someone seeing "blocklisted" and thinking it was a typo for "blacklisted" and undoing the change to comments with an NFC change.

However I can "disagree and commit" (cf. https://en.wikipedia.org/wiki/Disagree_and_commit) with this choice of wording. I can search out the awkward Blocklisted and come up with different phrasing in the prose, but I think names in the program should use blocklist so that they can be easily connected to the option name for future maintainers of llvm. I'll check this.

I agree that the frontend option names and the internal variable names need to connect to one another, so if we pick a new term, it should be used consistently. I'd be mildly happier with a better term than blocklist, but I can live with that name too.

Thank you for working on this!

This changes the option names that include substring blacklist to blocklist.

I think this change works in some places, but in other places we say things like "blocklisted" which feels a bit awkward. I don't super love the name blocklist, but I don't super hate it either. All the alternative names I come up with aren't really great either, like UnsanitiziedEntities or IgnoredObjects. Maybe someone will have a better idea here than me, but I do think the current name is an improvement over the old name.

There is a pre-existing patch https://reviews.llvm.org/D82244 using block and allow, that's why I chose blocklist. Although going in cold I would prefer to use allow and deny, and block seems to me a little like cheating (too similar to black).

That was my concern as well -- I can easily imagine someone seeing "blocklisted" and thinking it was a typo for "blacklisted" and undoing the change to comments with an NFC change.

However I can "disagree and commit" (cf. https://en.wikipedia.org/wiki/Disagree_and_commit) with this choice of wording. I can search out the awkward Blocklisted and come up with different phrasing in the prose, but I think names in the program should use blocklist so that they can be easily connected to the option name for future maintainers of llvm. I'll check this.

I agree that the frontend option names and the internal variable names need to connect to one another, so if we pick a new term, it should be used consistently. I'd be mildly happier with a better term than blocklist, but I can live with that name too.

FWIW I would prefer denylist as well. (Also uses of whitelist should be allowlist, but also incremental :)

-eric

Thank you for working on this!

This changes the option names that include substring blacklist to blocklist.

I think this change works in some places, but in other places we say things like "blocklisted" which feels a bit awkward. I don't super love the name blocklist, but I don't super hate it either. All the alternative names I come up with aren't really great either, like UnsanitiziedEntities or IgnoredObjects. Maybe someone will have a better idea here than me, but I do think the current name is an improvement over the old name.

There is a pre-existing patch https://reviews.llvm.org/D82244 using block and allow, that's why I chose blocklist. Although going in cold I would prefer to use allow and deny, and block seems to me a little like cheating (too similar to black).

That was my concern as well -- I can easily imagine someone seeing "blocklisted" and thinking it was a typo for "blacklisted" and undoing the change to comments with an NFC change.

However I can "disagree and commit" (cf. https://en.wikipedia.org/wiki/Disagree_and_commit) with this choice of wording. I can search out the awkward Blocklisted and come up with different phrasing in the prose, but I think names in the program should use blocklist so that they can be easily connected to the option name for future maintainers of llvm. I'll check this.

I agree that the frontend option names and the internal variable names need to connect to one another, so if we pick a new term, it should be used consistently. I'd be mildly happier with a better term than blocklist, but I can live with that name too.

FWIW I would prefer denylist as well. (Also uses of whitelist should be allowlist, but also incremental :)

-eric

I can change D82244 used blocklist to denylist . If there is no need for compatibility, I'll just replace the strings. If there is need for compatibility, I can make blocklist an alias.

FWIW I would prefer denylist as well. (Also uses of whitelist should be allowlist, but also incremental :)

I feel like existing and proposed value do match the meaning of this list.
maybe ignorelist, skiplist (can be confused with data structure?), or just asan_no_sanitize.txt or even no_asan.txt

I can change D82244 used blocklist to denylist . If there is no need for compatibility, I'll just replace the strings. If there is need for compatibility, I can make blocklist an alias.

I don't think we need compatibility for D82244. If we change clang, we can update D82244 to the same for consistency.

clang/unittests/Driver/SanitizerArgsTest.cpp
134–136

always/never?

FWIW I would prefer denylist as well. (Also uses of whitelist should be allowlist, but also incremental :)

I feel like existing and proposed value do match the meaning of this list.
maybe ignorelist, skiplist (can be confused with data structure?), or just asan_no_sanitize.txt or even no_asan.txt

Would you use ignorelist (skiplist) in the option name as well as the filename? Can you recommend a name for the antonym as well (i.e. replacement for whitelist)

I can change D82244 used blocklist to denylist . If there is no need for compatibility, I'll just replace the strings. If there is need for compatibility, I can make blocklist an alias.

I don't think we need compatibility for D82244. If we change clang, we can update D82244 to the same for consistency.

Yes I agree.

clang/unittests/Driver/SanitizerArgsTest.cpp
134–136

Can you say a few more works here? You mean use always as replacement for white, and never as replacement for black?

FWIW I would prefer denylist as well. (Also uses of whitelist should be allowlist, but also incremental :)

I feel like existing and proposed value do match the meaning of this list.
maybe ignorelist, skiplist (can be confused with data structure?), or just asan_no_sanitize.txt or even no_asan.txt

Would you use ignorelist (skiplist) in the option name as well as the filename? Can you recommend a name for the antonym as well (i.e. replacement for whitelist)

If we change that I would also replaced:
-list to -file to match other

We have attributes for a similar purpose in language:
https://clang.llvm.org/docs/AttributeReference.html#no-sanitize-address-no-address-safety-analysis
So maybe we can use that scheme for files which used for the same purpose?
-fno-sanitize-file=..../no_sanitize_address.txt

However we have -fno-sanitize-blocklist which then should be -fno-no-sanitize-file? :)
But I hope we can just remove that flag D79043

clang/include/clang/Driver/Options.td
1432–1455

This file has to many independent changes, I'd separate them into a separate patches when possible. At least user visible changes as this one and Inputs/resource_dir/share/ into a separate one.
Also it's easier to review.

clang/unittests/Driver/SanitizerArgsTest.cpp
134–136

This are internal variables of unittest, it can be anything
use use thatever is part of -fxray-*-instrument= flags

vitalybuka added inline comments.Feb 12 2021, 11:58 AM
clang/include/clang/Basic/SanitizerBlacklist.h
20 ↗(On Diff #321933)

This file should go into a separate patch. It touches multiple files with simple changes but unlikely will case rollback.
It's unrelated to most critical change in Options.td which may need some discussion.
Options.td so I am ready accept such changes here changes ASAP.

28 ↗(On Diff #321933)

NoSanitizeList ?

36 ↗(On Diff #321933)

containsGlobal
containsType

mibintc added inline comments.
clang/include/clang/Basic/SanitizerBlacklist.h
20 ↗(On Diff #321933)

I changed the filename in this patch, https://reviews.llvm.org/D96974

Could you please rebase the patch onto D96974

mibintc updated this revision to Diff 326412.Feb 25 2021, 8:57 AM

I rebased the patch and basically made no other changes. On the review there's a suggestion that 'blocklist' isn't the best word choice, do you have suggestions?

Exclusion list? or exclude?

Exclusion list? or exclude?

In 2a4317bfb319e, (Fangrui Song 2020-06-19) changed option name fsanitize-coverage-whitelist to fsanitize-coverage-allowlist. So another possibility is AllowList and DisallowList

To my taste "exclude" better represents meaning than "disallow".
E.g. disallow can be interpreted as "make them fatal". We can fix
2a4317bfb319e for consistency later.
-fsanitize-exclude-list= ?

vitalybuka requested changes to this revision.Apr 26 2021, 6:41 PM

Is this abandoned patch?

This revision now requires changes to proceed.Apr 26 2021, 6:41 PM
mibintc abandoned this revision.Apr 27 2021, 5:01 AM

There was no resolution about what option name would be acceptable