This is an archive of the discontinued LLVM Phabricator instance.

[PCH] make sure to not warn about unused macros from -D
ClosedPublic

Authored by llunak on Feb 2 2020, 3:24 AM.

Details

Summary

If a PCH is used for compilation, SourceManager::isInMainFile() returns true even for the "<built-in>" predefines area. Using -D only for the TU compilation may trigger -Wunused-macros for it. It is admitedly a bit fishy to set a macro only for a TU and not for the PCH, but this works fine if the PCH does not use the macro (I couldn't find a statement on this for Clang, but GCC explicitly allows this in the docs).

Diff Detail

Event Timeline

llunak created this revision.Feb 2 2020, 3:24 AM
lebedev.ri retitled this revision from make sure to not warn about unused macros from -D to [PCH] make sure to not warn about unused macros from -D.Mar 6 2020, 1:14 AM

Looks pretty reasonable to me - but I don't know enough about the macro and source location infrastructure. @rsmith - mind taking a quick look?

rsmith accepted this revision.Apr 19 2020, 12:37 PM

Looks OK as a workaround. Do you know why we consider these to be in the main file? If we could fix that in the source manager, that'd seem preferable.

This revision is now accepted and ready to land.Apr 19 2020, 12:37 PM

Looks OK as a workaround. Do you know why we consider these to be in the main file? If we could fix that in the source manager, that'd seem preferable.

According to my testing, SourceManager::isInMainFile() can handle "<built-in>" locations in 3 ways:

  • for macros defined using -D on the command line the control flow returns false in the "if (FI.hasLineDirectives())" block
  • for built-in macros such as clang the control flow enters the same "if (FI.hasLineDirectives())" block, but Entry->IncludeOffset is 0, so the flow then reaches the final "return FI.getIncludeLoc().isInvalid();", which returns true
  • if PCH is used, macros defined using -D on the command line do not even enter "if (FI.hasLineDirectives())" and so they end up returning true the same way built-in macros do

But I don't understand this enough to know why and what that actually means.

I've also tried a patch that added SourceManager::setPredefinesFileID() and moved the check from this patch to SourceManager::isInMainFile(), but then tests fail because apparently Preprocessor::setPredefinesFileID() may be called multiple times.

This revision was automatically updated to reflect the committed changes.