This is an archive of the discontinued LLVM Phabricator instance.

[ASan] Rename `-fsanitize-address-destructor-kind=` to drop the `-kind` suffix.
ClosedPublic

Authored by delcypher on Apr 28 2021, 2:32 PM.

Details

Summary

Renaming the option is based on discussions in https://reviews.llvm.org/D101122.

It is normally not a good idea to rename driver flags but this flag is
new enough and obscure enough that it is very unlikely to have adopters.

Diff Detail

Event Timeline

delcypher created this revision.Apr 28 2021, 2:32 PM
delcypher requested review of this revision.Apr 28 2021, 2:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2021, 2:32 PM

@jansvoboda11 Should I use the the alias feature so that the old -fsanitize-address-destructor-kind argument is still parsed? I'm not sure if it's worth it but if doing so is very simple and has a low maintenance burden we should probably do it.

vitalybuka accepted this revision.Apr 28 2021, 3:35 PM

Thanks!

clang/docs/ClangCommandLineReference.rst
873

<arc> here is inconsistent with MetaVarName<"<kind>"

This revision is now accepted and ready to land.Apr 28 2021, 3:35 PM
delcypher updated this revision to Diff 341384.Apr 28 2021, 7:02 PM

Remove metavar

jansvoboda11 accepted this revision.Apr 29 2021, 5:20 AM

@jansvoboda11 Should I use the the alias feature so that the old -fsanitize-address-destructor-kind argument is still parsed? I'm not sure if it's worth it but if doing so is very simple and has a low maintenance burden we should probably do it.

I'm fine with omitting the alias if the original flag didn't make it into a release and it's unlikely that downstream TOT users are using it.

clang/include/clang/Driver/Options.td
1541–1542

Nit: remove _kind from the def name.

@jansvoboda11 Should I use the the alias feature so that the old -fsanitize-address-destructor-kind argument is still parsed? I'm not sure if it's worth it but if doing so is very simple and has a low maintenance burden we should probably do it.

I'm fine with omitting the alias if the original flag didn't make it into a release and it's unlikely that downstream TOT users are using it.

The change that landed this flag originally (5d64dd8e3c22e12e4f7b3d08ffe88fc41e727117) doesn't seem to be in the release/12.x branch. It also wasn't cherry picked to the downstream Apple branches on GitHub. I can't be sure about other downstream users of LLVM but I do think it's very unlikely that anyone adopted this flag.

I don't mind adding an alias but I'd need a little guidance on how to do it correctly and also where to add code to emit a warning about the old flag being deprecated.

clang/docs/ClangCommandLineReference.rst
873

I'll fix that now.

clang/include/clang/Driver/Options.td
1541–1542

Good catch. I'll fix that.

Rename def too.

@jansvoboda11 I'm going to land this patch as is (with your nit fixed). If you would like me to add an alias please follow up with me and I can put up a patch to do that.

This revision was landed with ongoing or failed builds.Apr 29 2021, 11:56 AM
This revision was automatically updated to reflect the committed changes.