[llvm] r333091 - Remove DEBUG macro.
Daniel Sanders via llvm-commits
llvm-commits at lists.llvm.org
Fri May 25 10:14:58 PDT 2018
> On 25 May 2018, at 03:45, Nicola Zaghen <nicola.zaghen at imgtec.com> wrote:
>
> Hi Daniel,
>
> The removal was discussed on the mailing list back in March http://lists.llvm.org/pipermail/llvm-dev/2018-March/122097.html <http://lists.llvm.org/pipermail/llvm-dev/2018-March/122097.html>, but you make a good point: an email to let people know that the change is actually in and with some instructions would be a good thing to do. I'll look into it now.
>
Ah, ok. I must have missed it while I was catching up after GDC.
> Regarding the quick replace -> remove, this was also part of the email (and also the review of the change). It was done so quickly because every day people would add new uses of the DEBUG() macro as it would have kept working the same way, which would then left us with two equivalent macros for a long time.
>
I agree that if it's there and usable then it will sneak back in. We could potentially have made the old macro emit deprecation warnings. It's not possible to mark a macro as being deprecated but it's possible to make it call a deprecated function like so:
diff --git a/include/llvm/Support/Debug.h b/include/llvm/Support/Debug.h
index 980abfb0e8d..86c6131604d 100644
--- a/include/llvm/Support/Debug.h
+++ b/include/llvm/Support/Debug.h
@@ -29,6 +29,8 @@
#ifndef LLVM_SUPPORT_DEBUG_H
#define LLVM_SUPPORT_DEBUG_H
+#include "llvm/Support/Compiler.h"
+
namespace llvm {
class raw_ostream;
@@ -118,6 +120,9 @@ raw_ostream &dbgs();
//
#define LLVM_DEBUG(X) DEBUG_WITH_TYPE(DEBUG_TYPE, X)
+LLVM_ATTRIBUTE_DEPRECATED(void deprecate_debug_macro(), "Use LLVM_DEBUG instead");
+#define DEBUG(X) do { deprecate_debug_macro(); LLVM_DEBUG(X); } while(false)
+
} // end namespace llvm
#endif // LLVM_SUPPORT_DEBUG_H
I don't think it makes sense to re-introduce it in order to deprecate it though.
> With regards to the sed command I provided, I thought sed would be "fairly recent" on all systems, but it seems like the version shipped with macOS is quite dated (or at least lacks features).
>
It's a BSD sed so it lacks quite a few things from GNU sed. I run into quirks like this fairly often. I found the mandatory argument to -i particularly confusing when I first encountered it :-)
> I tried rewriting it with awk but I had the same issue.
> Currently I have this perl one which worked fine on both Linux (Ubuntu 16.04) and macOS 10.13 with a fresh git clone of llvm-mirror.
>
> git grep -l 'DEBUG' | xargs perl -pi -e 's/\bDEBUG\s?\(/LLVM_DEBUG(/g'
>
> Can you test it out and let me know if it works for you? I'd like to have one that works on multiple platforms so I can eventually update the release notes and email llvm-dev with it (in the "change has been completed" email).
>
That works for me. Using that and the clang-format command, I get the same result as I did with the macOS sed command.
Thanks
>
> Thanks,
> Nicola
>
> On 24/05/18 17:43, Daniel Sanders wrote:
>> Hi Nicola,
>>
>> This removal should get an email on llvm-dev and a mention in the release notes since there's a lot of out-of-tree projects that are going to be broken by this change and won't have noticed the llvm-commits post. In my case I can't find the earlier commits that added LLVM_DEBUG in my mailbox so I'm not sure what happened there, maybe it was filtered out for being too large. Some projects will notice when they're pulling the trunk into their downstream repo's (like we did :-) ) but there will also be another wave when we do the next release as there's also out of tree targets that don't closely track trunk.
>>
>> It's also worth mentioning the merge pain that the very quick removal is going to cause. I'm reluctant to mention this since I believe it isn't LLVM's responsibility to account for it (it's ours) but this case is a bit special in that it's likely to hit an unusually large number of merges over the next few years. As bugs are fixed in existing releases, it's common to merge those fixes into the current code-base so that the bugs aren't introduced again. Effectively that means that some branches are using DEBUG() and others are using LLVM_DEBUG() and the practical result of this is that any merges containing or near lines with the DEBUG() macro are going result in merge conflicts. Probably the least painful way to resolve it is to re-introduce DEBUG() locally but I don't like that solution because we'll never make the switch. Unfortunately making the switch on all branches probably isn't an option either. I'm going to have a think on how best to deal with this.
>>
>> By the way, the sed command from your earlier commit doesn't work on macOS because -i has a mandatory argument and \b isn't supported. Even with those issues fixed I end up with:
>> sed: RE error: illegal byte sequence
>>
>> This seems to work though:
>> for f in $(grep -rlw --exclude-dir=.git 'DEBUG' .);
>> do
>> echo $f
>> sed -i '.bak' -e 's/[[:<:]]DEBUG[[:>:]][ \t]*(/LLVM_DEBUG(/g' $f;
>> done
>>
>>> On 23 May 2018, at 08:09, Nicola Zaghen via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>>
>>> Author: nzaghen
>>> Date: Wed May 23 08:09:29 2018
>>> New Revision: 333091
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=333091&view=rev <http://llvm.org/viewvc/llvm-project?rev=333091&view=rev>
>>> Log:
>>> Remove DEBUG macro.
>>>
>>> Now that the LLVM_DEBUG() macro landed on the various sub-projects
>>> the DEBUG macro can be removed.
>>> Also change the new uses of DEBUG to LLVM_DEBUG.
>>>
>>> Differential Revision: https://reviews.llvm.org/D46952 <https://reviews.llvm.org/D46952>
>>>
>>>
>>> Modified:
>>> llvm/trunk/include/llvm/DebugInfo/PDB/DIA/DIASupport.h
>>> llvm/trunk/include/llvm/Support/Debug.h
>>> llvm/trunk/lib/Target/X86/X86DomainReassignment.cpp
>>> llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp
>>> llvm/trunk/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
>>> llvm/trunk/lib/Transforms/Vectorize/VPlanVerifier.cpp
>>> llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp
>>>
>>> Modified: llvm/trunk/include/llvm/DebugInfo/PDB/DIA/DIASupport.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo/PDB/DIA/DIASupport.h?rev=333091&r1=333090&r2=333091&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo/PDB/DIA/DIASupport.h?rev=333091&r1=333090&r2=333091&view=diff>
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/DebugInfo/PDB/DIA/DIASupport.h (original)
>>> +++ llvm/trunk/include/llvm/DebugInfo/PDB/DIA/DIASupport.h Wed May 23 08:09:29 2018
>>> @@ -22,14 +22,6 @@
>>> #define NOMINMAX
>>> #endif
>>>
>>> -// llvm/Support/Debug.h unconditionally #defines DEBUG as a macro.
>>> -// DIA headers #define it if it is not already defined, so we have
>>> -// an order of includes problem. The real fix is to make LLVM use
>>> -// something less generic than DEBUG, such as LLVM_DEBUG(), but it's
>>> -// fairly prevalent. So for now, we save the definition state and
>>> -// restore it.
>>> -#pragma push_macro("DEBUG")
>>> -
>>> // atlbase.h has to come before windows.h
>>> #include <atlbase.h>
>>> #include <windows.h>
>>> @@ -39,6 +31,4 @@
>>> #include <dia2.h>
>>> #include <diacreate.h>
>>>
>>> -#pragma pop_macro("DEBUG")
>>> -
>>> #endif // LLVM_DEBUGINFO_PDB_DIA_DIASUPPORT_H
>>>
>>> Modified: llvm/trunk/include/llvm/Support/Debug.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Debug.h?rev=333091&r1=333090&r2=333091&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Debug.h?rev=333091&r1=333090&r2=333091&view=diff>
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/Support/Debug.h (original)
>>> +++ llvm/trunk/include/llvm/Support/Debug.h Wed May 23 08:09:29 2018
>>> @@ -118,8 +118,6 @@ raw_ostream &dbgs();
>>> //
>>> #define LLVM_DEBUG(X) DEBUG_WITH_TYPE(DEBUG_TYPE, X)
>>>
>>> -#define DEBUG(X) LLVM_DEBUG(X)
>>> -
>>> } // end namespace llvm
>>>
>>> #endif // LLVM_SUPPORT_DEBUG_H
>>>
>>> Modified: llvm/trunk/lib/Target/X86/X86DomainReassignment.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86DomainReassignment.cpp?rev=333091&r1=333090&r2=333091&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86DomainReassignment.cpp?rev=333091&r1=333090&r2=333091&view=diff>
>>> ==============================================================================
>>> --- llvm/trunk/lib/Target/X86/X86DomainReassignment.cpp (original)
>>> +++ llvm/trunk/lib/Target/X86/X86DomainReassignment.cpp Wed May 23 08:09:29 2018
>>> @@ -753,7 +753,7 @@ bool X86DomainReassignment::runOnMachine
>>> }
>>>
>>> for (Closure &C : Closures) {
>>> - DEBUG(C.dump(MRI));
>>> + LLVM_DEBUG(C.dump(MRI));
>>> if (isReassignmentProfitable(C, MaskDomain)) {
>>> reassign(C, MaskDomain);
>>> ++NumClosuresConverted;
>>>
>>> Modified: llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp?rev=333091&r1=333090&r2=333091&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp?rev=333091&r1=333090&r2=333091&view=diff>
>>> ==============================================================================
>>> --- llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp (original)
>>> +++ llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp Wed May 23 08:09:29 2018
>>> @@ -645,8 +645,8 @@ void MergeFunctions::filterInstsUnrelate
>>> static bool isThunkProfitable(Function * F) {
>>> if (F->size() == 1) {
>>> if (F->front().size() <= 2) {
>>> - DEBUG(dbgs() << "isThunkProfitable: " << F->getName()
>>> - << " is too small to bother creating a thunk for\n");
>>> + LLVM_DEBUG(dbgs() << "isThunkProfitable: " << F->getName()
>>> + << " is too small to bother creating a thunk for\n");
>>> return false;
>>> }
>>> }
>>>
>>> Modified: llvm/trunk/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp?rev=333091&r1=333090&r2=333091&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp?rev=333091&r1=333090&r2=333091&view=diff>
>>> ==============================================================================
>>> --- llvm/trunk/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp (original)
>>> +++ llvm/trunk/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp Wed May 23 08:09:29 2018
>>> @@ -116,7 +116,7 @@ VPBasicBlock *PlainCFGBuilder::getOrCrea
>>> return BlockIt->second;
>>>
>>> // Create new VPBB.
>>> - DEBUG(dbgs() << "Creating VPBasicBlock for " << BB->getName() << "\n");
>>> + LLVM_DEBUG(dbgs() << "Creating VPBasicBlock for " << BB->getName() << "\n");
>>> VPBasicBlock *VPBB = new VPBasicBlock(BB->getName());
>>> BB2VPBB[BB] = VPBB;
>>> VPBB->setParent(TopRegion);
>>> @@ -314,7 +314,7 @@ void VPlanHCFGBuilder::buildHierarchical
>>> PlainCFGBuilder PCFGBuilder(TheLoop, LI, Plan);
>>> VPRegionBlock *TopRegion = PCFGBuilder.buildPlainCFG();
>>> Plan.setEntry(TopRegion);
>>> - DEBUG(Plan.setName("HCFGBuilder: Plain CFG\n"); dbgs() << Plan);
>>> + LLVM_DEBUG(Plan.setName("HCFGBuilder: Plain CFG\n"); dbgs() << Plan);
>>>
>>> Verifier.verifyHierarchicalCFG(TopRegion);
>>> }
>>>
>>> Modified: llvm/trunk/lib/Transforms/Vectorize/VPlanVerifier.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectorize/VPlanVerifier.cpp?rev=333091&r1=333090&r2=333091&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectorize/VPlanVerifier.cpp?rev=333091&r1=333090&r2=333091&view=diff>
>>> ==============================================================================
>>> --- llvm/trunk/lib/Transforms/Vectorize/VPlanVerifier.cpp (original)
>>> +++ llvm/trunk/lib/Transforms/Vectorize/VPlanVerifier.cpp Wed May 23 08:09:29 2018
>>> @@ -120,7 +120,7 @@ void VPlanVerifier::verifyHierarchicalCF
>>> if (!EnableHCFGVerifier)
>>> return;
>>>
>>> - DEBUG(dbgs() << "Verifying VPlan H-CFG.\n");
>>> + LLVM_DEBUG(dbgs() << "Verifying VPlan H-CFG.\n");
>>> assert(!TopRegion->getParent() && "VPlan Top Region should have no parent.");
>>> verifyRegionRec(TopRegion);
>>> }
>>>
>>> Modified: llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp?rev=333091&r1=333090&r2=333091&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp?rev=333091&r1=333090&r2=333091&view=diff>
>>> ==============================================================================
>>> --- llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp (original)
>>> +++ llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp Wed May 23 08:09:29 2018
>>> @@ -4063,7 +4063,7 @@ std::vector<Matcher *> GlobalISelEmitter
>>> }
>>> ProcessCurrentGroup();
>>>
>>> - DEBUG(dbgs() << "NumGroups: " << NumGroups << "\n");
>>> + LLVM_DEBUG(dbgs() << "NumGroups: " << NumGroups << "\n");
>>> assert(CurrentGroup->empty() && "The last group wasn't properly processed");
>>> return OptRules;
>>> }
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180525/76c2ca85/attachment-0001.html>
More information about the llvm-commits
mailing list