This is an archive of the discontinued LLVM Phabricator instance.

Change DEBUG() macro to LLVM_DEBUG()
ClosedPublic

Authored by Nicola on Feb 22 2018, 8:29 AM.

Details

Summary

The DEBUG() macro is too generic so it might clash with other projects.
I noticed in include/llvm/DebugInfo/PDB/DIA/DIASupport.h a comment expressing interest in changing DEBUG() to LLVM_DEBUG() so I decided to give it a go.

This is the command I used to do the replacement:
git grep -l 'DEBUG' | xargs sed -i 's/\bDEBUG\s\?(/LLVM_DEBUG(/g'

Then I manually adjusted the DEBUG macro push/pop uses in APInt.cpp and DIASupport.h
Also, formatted the patch with git diff -U0 master | ../clang/tools/clang-format/clang-format-diff.py -i -p1 -style LLVM

Diff Detail

Repository
rL LLVM

Event Timeline

Nicola created this revision.Feb 22 2018, 8:29 AM
1ace added a subscriber: 1ace.Feb 23 2018, 6:08 AM
sbc100 added inline comments.Feb 27 2018, 3:42 PM
lib/MC/WasmObjectWriter.cpp
950 ↗(On Diff #135422)

Maybe you want to git clang-format this to avoid introducing lines that are more than 80 cols?

sanjoy removed a subscriber: sanjoy.Feb 28 2018, 9:44 PM
Nicola updated this revision to Diff 136490.EditedMar 1 2018, 3:30 AM
Nicola edited the summary of this revision. (Show Details)

Reformatted the patch with clang-format-diff. Also rebased against latest LLVM master.

Ping ping?
clang is also using the DEBUG() macro and other projects might be doing the same. Any suggestion on how to proceed?

zturner accepted this revision.Mar 15 2018, 9:46 AM

Sorry for dragging my feet on reviewing this. I know it's a pain to keep large patches like this properly rebased. LGTM, I think this review has been up for enough time that I don't think anyone else is going to have any objections.

This revision is now accepted and ready to land.Mar 15 2018, 9:46 AM
Nicola updated this revision to Diff 138707.Mar 16 2018, 8:15 AM

Updated the patch to trunk@327699. I don't have commit rights so I cannot submit it, can you do it?

Should I open a review to change the DEBUG() macro in clang too? They use the LLVM one, so not having it will introduce build breaks. The patch for them should be quite small.

kuhar added a comment.Mar 16 2018, 8:39 AM

Updated the patch to trunk@327699. I don't have commit rights so I cannot submit it, can you do it?

Should I open a review to change the DEBUG() macro in clang too? They use the LLVM one, so not having it will introduce build breaks. The patch for them should be quite small.

Note that there are much more (sub)projects within LLVM that use the DEBUG macro the patch shouldn't break. There are also countless downstream projects that will be affected and some other people will have to refactor them.

The DEBUG() macro is too generic so it might clash with other projects.

I understand the argument and agree in general, but are there some known problems with such clashes? I think this patch is going to cause quite a hassle to many users and I would like to understand the motivation better. Maybe it would be a better idea to explain the situation in more detail and post it as an RFC on llvm-dev.

Have you thought of leaving the old DEBUG macro around and making it an alias for the new LLVM_DEBUG? The old macro can be considered deprecated for some time before it gets completely removed, but this way the could give users some time to transition instead of breaking everything at once.

include/llvm/Support/Debug.h
15 ↗(On Diff #138707)

I think this formatting should be fixed manually

Updated the patch to trunk@327699. I don't have commit rights so I cannot submit it, can you do it?

Should I open a review to change the DEBUG() macro in clang too? They use the LLVM one, so not having it will introduce build breaks. The patch for them should be quite small.

Note that there are much more (sub)projects within LLVM that use the DEBUG macro the patch shouldn't break. There are also countless downstream projects that will be affected and some other people will have to refactor them.

I understand the concern, that's why I checked the "main" projects: libcxx, lld, compiler-rt and lldb and none of them use the macro. clang is the only one that does (as far as I can tell)
Which other projects should I be checking?

The DEBUG() macro is too generic so it might clash with other projects.

I understand the argument and agree in general, but are there some known problems with such clashes? I think this patch is going to cause quite a hassle to many users and I would like to understand the motivation better. Maybe it would be a better idea to explain the situation in more detail and post it as an RFC on llvm-dev.

There are (were, at least?) clashes with other open source projects, such as MESA: https://lists.freedesktop.org/archives/mesa-dev/2016-July/124111.html and they ended up renaming DEBUG into MESA_DEBUG to avoid clashing with LLVM.
Thanks for pointing out about the RFC, I didn't think about it. I'll write up a post for llvm-dev in the coming days.

Have you thought of leaving the old DEBUG macro around and making it an alias for the new LLVM_DEBUG? The old macro can be considered deprecated for some time before it gets completely removed, but this way the could give users some time to transition instead of breaking everything at once.

I did think about leaving the old DEBUG macro around, that would make things easier on the breakage side. The issue is that we would need a way to deprecate the macro so that people stop using it and switch to the new one.
We could have a transition period in which we have both macros, but the "old" DEBUG macro should eventually be removed.

kuhar added a comment.Mar 16 2018, 9:31 AM

I understand the concern, that's why I checked the "main" projects: libcxx, lld, compiler-rt and lldb and none of them use the macro. clang is the only one that does (as far as I can tell)
Which other projects should I be checking?

From the top of my head definitely Polly uses it. I'd make sure by cloning the git monorepo and doing a quick grep over it.

There are (were, at least?) clashes with other open source projects, such as MESA: https://lists.freedesktop.org/archives/mesa-dev/2016-July/124111.html and they ended up renaming DEBUG into MESA_DEBUG to avoid clashing with LLVM.

I see, thank you for the link.

I did think about leaving the old DEBUG macro around, that would make things easier on the breakage side. The issue is that we would need a way to deprecate the macro so that people stop using it and switch to the new one.
We could have a transition period in which we have both macros, but the "old" DEBUG macro should eventually be removed.

Well, worst case we could just announce it deprecated on the mailing list.

nhaehnle removed a subscriber: nhaehnle.Mar 17 2018, 5:26 AM
bcain added a subscriber: bcain.Mar 23 2018, 1:54 PM
Nicola updated this revision to Diff 139809.Mar 26 2018, 9:50 AM

Rebased up to trunk@328503. Addressed comments about formatting on Debug.h (comment formatting is now fixed up manually)
This is still mostly with the commands I posted earlier (aside of Debug.h, DIASupport.h and APInt.cpp).

I kept the old DEBUG() macro around for a short transitional period. I'll open reviews with clang and Polly soon to remove the remaining DEBUG macro uses. Then I'll get a new review started for the final removal.

Regression tests passed on my side, but I don't have commit rights to submit the patch.
Are there any more concerns that need addressing? Otherwise I'm OK with it being submitted anytime.

sabuasal removed a subscriber: sabuasal.Mar 26 2018, 2:14 PM

I understand the concern, that's why I checked the "main" projects: libcxx, lld, compiler-rt and lldb and none of them use the macro. clang is the only one that does (as far as I can tell)
Which other projects should I be checking?

From the top of my head definitely Polly uses it. I'd make sure by cloning the git monorepo and doing a quick grep over it.

I do confirm that we are using DEBUG(dbgs() << ./..) in Polly.

Submitted separate reviews for the other projects that use the macro:
clang: https://reviews.llvm.org/D44975
clang-tools-extra: https://reviews.llvm.org/D44976
lld: https://reviews.llvm.org/D44977
Polly: https://reviews.llvm.org/D44978

Updating the LLVM patch up to r328689 now

Nicola updated this revision to Diff 140062.Mar 28 2018, 3:48 AM
This revision was automatically updated to reflect the committed changes.