[cfe-commits] [PATCH] Support for pragma include_alias
Richard Smith
richard at metafoo.co.uk
Thu Mar 1 17:46:46 PST 2012
On Thu, Mar 1, 2012 at 10:43 AM, Aaron Ballman <aaron at aaronballman.com>wrote:
> On Thu, Mar 1, 2012 at 12:21 AM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > Hi,
> >
> >> > On Sun, Feb 19, 2012 at 5:31 PM, Aaron Ballman <
> aaron at aaronballman.com>
> >> > wrote:
> >> >> One of the MSVC pragmas is the ability to remap include filenames, as
> >> >> described in MSDN:
> >> >> http://msdn.microsoft.com/en-us/library/wbeh5h91.aspx
> >> >>
> >> >> The long and short of it is: you can use this pragma to map the
> source
> >> >> filename to a replacement filename (usually for supporting the old
> 8.3
> >> >> filenames on FAT systems, but can be used arbitrarily). For
> instance:
> >> >>
> >> >> #pragma include_alias( "FoobarIsReallyLong.h", "FOOBAR~1.H" )
> >> >> #include "FoobarIsReallyLong.h" // Actually opens up FOOBAR~1.H
> >> >>
> >> >> #pragma include_alias( <foo.h>, <bar.h> )
> >> >> #include <foo.h> // Actually includes <bar.h>
> >> >> #include "foo.h" // Still includes "foo.h"
> >> >>
> >> >> This patch implements that pragma, and closes Bug 10705.
> >
> >
> > Sorry for the delay!
> >
> >> Index: include/clang/Basic/DiagnosticLexKinds.td
> >> ===================================================================
> >> --- include/clang/Basic/DiagnosticLexKinds.td (revision 150897)
> >> +++ include/clang/Basic/DiagnosticLexKinds.td (working copy)
> >> @@ -292,6 +292,15 @@
> >> ExtWarn<"__has_warning expected option name (e.g. \"-Wundef\")">,
> >> InGroup<MalformedWarningCheck>;
> >>
> >> +def warn_pragma_include_alias_mismatch :
> >> + ExtWarn<"pragma include_alias requires matching include directives "
> >> + "(e.g include_alias(\"foo.h\", \"bar.h\") or "
> >> + "include_alias(<foo.h>, <bar.h>))">,
> >> + InGroup<UnknownPragmas>;
> >
> > Can we make this diagnostic clearer? I'd prefer for us to produce
> different
> > text depending on whether the source include was a "include" or a
> <include>.
> > As a first pass:
> >
> > angle-bracketed include <%0> cannot be aliased to double-quoted include
> "%1"
> > double-quoted include "%0" cannot be aliased to angle-bracketed include
> <%1>
> >
> >> +def warn_pragma_include_alias_expected :
> >> + ExtWarn<"pragma include_alias expected '%0'">,
> >> + InGroup<UnknownPragmas>;
> >> +
> >> def err__Pragma_malformed : Error<
> >> "_Pragma takes a parenthesized string literal">;
> >> def err_pragma_comment_malformed : Error<
> >> Index: include/clang/Lex/HeaderSearch.h
> >> ===================================================================
> >> --- include/clang/Lex/HeaderSearch.h (revision 150897)
> >> +++ include/clang/Lex/HeaderSearch.h (working copy)
> >> @@ -154,6 +154,9 @@
> >> llvm::StringMap<const DirectoryEntry *, llvm::BumpPtrAllocator>
> >> FrameworkMap;
> >>
> >> + llvm::StringMap<std::pair<StringRef, bool>, llvm::BumpPtrAllocator>
> >> + IncludeAliasMap;
> >
> > This map doesn't appear to be sufficient: the Microsoft documentation
> > indicates that "foo" and <foo> can be aliased to different includes.
> >
> > Also, the StringRef in the value type here will refer to a string whose
> > lifetime has ended.
> >
> >> +
> >> /// HeaderMaps - This is a mapping from FileEntry -> HeaderMap,
> >> uniquing
> >> /// headermaps. This vector owns the headermap.
> >> std::vector<std::pair<const FileEntry*, const HeaderMap*> >
> HeaderMaps;
> >> @@ -217,6 +220,25 @@
> >> SystemDirIdx++;
> >> }
> >>
> >> + /// AddHeaderMapping -- Map the source include name to the dest
> include
> >> name
> >> + void AddHeaderMapping(const StringRef& Source, const StringRef& Dest,
> >
> > Generally, please write " &" not "& ". Also, StringRefs are usually
> passed
> > by value.
> >
> >> + bool IsAngled) {
> >> + IncludeAliasMap[Source] = std::make_pair(Dest, IsAngled);
> >> + }
> >> +
> >> + StringRef MapHeader(const StringRef& Source, bool isAngled) {
> >> + // Do any filename replacements before anything else
> >> + llvm::StringMap<std::pair<StringRef,bool>>::const_iterator iter =
> >
> > Please capitalize IsAngled and Iter, and write "> >" not ">>". I don't
> like
> > repeating this type here (and especially not missing out the
> > llvm::BumpPtrAllocator argument); consider adding a typedef for your
> > container type.
> >
> >> + IncludeAliasMap.find(Source);
> >> + if (iter != IncludeAliasMap.end()) {
> >> + // If the angling matches, then we've found a replacement
> >> + if (iter->second.second == isAngled) {
> >> + return iter->second.first;
> >> + }
> >
> > Prevailing llvm/clang style is to omit the braces around a simple
> > single-statement body such as this.
> >
> >> + }
> >> + return Source;
> >> + }
> >> +
> >> /// \brief Set the path to the module cache.
> >> void setModuleCachePath(StringRef CachePath) {
> >> ModuleCachePath = CachePath;
> >> Index: include/clang/Lex/Preprocessor.h
> >> ===================================================================
> >> --- include/clang/Lex/Preprocessor.h (revision 150897)
> >> +++ include/clang/Lex/Preprocessor.h (working copy)
> >> @@ -1262,6 +1262,7 @@
> >> void HandlePragmaMessage(Token &MessageTok);
> >> void HandlePragmaPushMacro(Token &Tok);
> >> void HandlePragmaPopMacro(Token &Tok);
> >> + void HandlePragmaIncludeAlias(Token &Tok);
> >> IdentifierInfo *ParsePragmaPushOrPopMacro(Token &Tok);
> >>
> >> // Return true and store the first token only if any CommentHandler
> >> Index: lib/Lex/PPDirectives.cpp
> >> ===================================================================
> >> --- lib/Lex/PPDirectives.cpp (revision 150897)
> >> +++ lib/Lex/PPDirectives.cpp (working copy)
> >> @@ -1297,6 +1297,9 @@
> >> PragmaARCCFCodeAuditedLoc = SourceLocation();
> >> }
> >>
> >> + // Map the filename
> >> + Filename = HeaderInfo.MapHeader(Filename, isAngled);
> >> +
> >> // Search include directories.
> >> const DirectoryLookup *CurDir;
> >> SmallString<1024> SearchPath;
> >> Index: lib/Lex/Pragma.cpp
> >> ===================================================================
> >> --- lib/Lex/Pragma.cpp (revision 150897)
> >> +++ lib/Lex/Pragma.cpp (working copy)
> >> @@ -663,6 +663,101 @@
> >> }
> >> }
> >>
> >> +void Preprocessor::HandlePragmaIncludeAlias(Token& Tok) {
> >> + // We will either get a quoted filename or a bracketed filename, and
> >> we
> >> + // have to track which we got. The first filename is the source
> name,
> >> + // and the second name is the mapped filename. If the first is
> quoted,
> >> + // the second must be as well (cannot mix and match quotes and
> >> brackets).
> >> + SourceLocation Loc = Tok.getLocation();
> >> +
> >> + // Get the open paren
> >> + Lex(Tok);
> >> + if (Tok.isNot(tok::l_paren)) {
> >> + Diag(Tok, diag::warn_pragma_include_alias_expected) << "(";
> >> + return;
> >> + }
> >> +
> >> + // We expect either a quoted string literal, or a bracketed name
> >> + Token SourceFilenameTok;
> >> + CurPPLexer->LexIncludeFilename(SourceFilenameTok);
> >> + if (SourceFilenameTok.is(tok::eod)) {
> >> + // The diagnostic has already been handled
> >> + return;
> >> + }
> >> +
> >> + StringRef SourceFileName;
> >> + SmallString<128> FileNameBuffer;
> >> + if (SourceFilenameTok.is(tok::string_literal) ||
> >> + SourceFilenameTok.is(tok::angle_string_literal)) {
> >> + SourceFileName = getSpelling(SourceFilenameTok, FileNameBuffer);
> >> + } else if (SourceFilenameTok.is(tok::less)) {
> >> + // This could be a path instead of just a name
> >> + FileNameBuffer.push_back('<');
> >> + SourceLocation End;
> >> + if (ConcatenateIncludeName(FileNameBuffer, End))
> >> + return; // Diagnostic already emitted
> >> + SourceFileName = FileNameBuffer.str();
> >> + } else {
> >> + Diag(Tok, diag::warn_pragma_include_alias_expected) << "include
> >> filename";
> >
> > English phrases shouldn't be used as diagnostic arguments, since they
> won't
> > be translatable (if we ever choose to translate the diagnostics). Please
> use
> > a different diagnostic for this; perhaps diag::err_pp_expects_filename?
> >
> > Also, I don't see a test for this error.
> >
> >> + return;
> >> + }
> >> + FileNameBuffer.clear();
> >> +
> >> + // Now we expect a comma, followed by another include name
> >> + Lex(Tok);
> >> + if (Tok.isNot(tok::comma)) {
> >> + Diag(Tok, diag::warn_pragma_include_alias_expected) << ",";
> >> + return;
> >> + }
> >> +
> >> + Token ReplaceFilenameTok;
> >> + CurPPLexer->LexIncludeFilename(ReplaceFilenameTok);
> >> + if (ReplaceFilenameTok.is(tok::eod)) {
> >> + // The diagnostic has already been handled
> >> + return;
> >> + }
> >> +
> >> + StringRef ReplaceFileName;
> >> + if (ReplaceFilenameTok.is(tok::string_literal) ||
> >> + ReplaceFilenameTok.is(tok::angle_string_literal)) {
> >> + ReplaceFileName = getSpelling(ReplaceFilenameTok, FileNameBuffer);
> >> + } else if (ReplaceFilenameTok.is(tok::less)) {
> >> + // This could be a path instead of just a name
> >> + FileNameBuffer.push_back('<');
> >> + SourceLocation End;
> >> + if (ConcatenateIncludeName(FileNameBuffer, End))
> >> + return; // Diagnostic already emitted
> >> + ReplaceFileName = FileNameBuffer.str();
> >> + } else {
> >> + Diag(Tok, diag::warn_pragma_include_alias_expected) << "include
> >> filename";
> >> + return;
> >> + }
> >
> > Could this repeated code be factored out? (I imagine we have largely the
> > same pattern in some other places too.)
> >
> >> +
> >> + // Finally, we expect the closing paren
> >> + Lex(Tok);
> >> + if (Tok.isNot(tok::r_paren)) {
> >> + Diag(Tok, diag::warn_pragma_include_alias_expected) << ")";
> >> + return;
> >> + }
> >> +
> >> + // Now that we have the source and target filenames, we need to make
> >> sure
> >> + // they're both of the same type (angled vs non-angled)
> >> + bool SourceIsAngled =
> >> + GetIncludeFilenameSpelling(SourceFilenameTok.getLocation(),
> >> + SourceFileName);
> >> + bool ReplaceIsAngled =
> >> + GetIncludeFilenameSpelling(ReplaceFilenameTok.getLocation(),
> >> + ReplaceFileName);
> >> + if (SourceIsAngled != ReplaceIsAngled) {
> >> + Diag(Loc, diag::warn_pragma_include_alias_mismatch);
> >> + return;
> >> + }
> >> +
> >> + // Now we can let the include handler know about this mapping
> >> + getHeaderSearchInfo().AddHeaderMapping(SourceFileName,
> >> ReplaceFileName,
> >
> > ReplaceFileName here refers to a string which expires at the end of this
> > function, and it's being saved away by AddHeaderMapping for later use.
> >
> >> + SourceIsAngled);
> >> +}
> >> +
> >> /// AddPragmaHandler - Add the specified pragma handler to the
> >> preprocessor.
> >> /// If 'Namespace' is non-null, then it is a token required to exist on
> >> the
> >> /// pragma line before the pragma string starts, e.g. "STDC" or "GCC".
> >> @@ -943,6 +1038,15 @@
> >> }
> >> };
> >>
> >> +/// PragmaIncludeAliasHandler - "#pragma include_alias("...")".
> >> +struct PragmaIncludeAliasHandler : public PragmaHandler {
> >> + PragmaIncludeAliasHandler() : PragmaHandler("include_alias") {}
> >> + virtual void HandlePragma(Preprocessor &PP, PragmaIntroducerKind
> >> Introducer,
> >> + Token &IncludeAliasTok) {
> >> + PP.HandlePragmaIncludeAlias(IncludeAliasTok);
> >> + }
> >> +};
> >> +
> >> /// PragmaMessageHandler - "#pragma message("...")".
> >> struct PragmaMessageHandler : public PragmaHandler {
> >> PragmaMessageHandler() : PragmaHandler("message") {}
> >> @@ -1095,5 +1199,6 @@
> >> // MS extensions.
> >> if (Features.MicrosoftExt) {
> >> AddPragmaHandler(new PragmaCommentHandler());
> >> + AddPragmaHandler(new PragmaIncludeAliasHandler());
> >> }
> >> }
> >> Index: test/Preprocessor/pragma_microsoft.c
> >> ===================================================================
> >> --- test/Preprocessor/pragma_microsoft.c (revision 150897)
> >> +++ test/Preprocessor/pragma_microsoft.c (working copy)
> >> @@ -38,3 +38,20 @@
> >> // this warning should go away.
> >> MACRO_WITH__PRAGMA // expected-warning {{expression result unused}}
> >> }
> >> +
> >> +
> >> +// This should include macro_arg_directive even though the include
> >> +// is looking for test.h This allows us to assign to "n"
> >> +#pragma include_alias("test.h", "macro_arg_directive.h" )
> >> +#include "test.h"
> >> +void test( void ) {
> >> + n = 12;
> >> +}
> >> +
> >> +#pragma include_alias("foo.h", <bar.h>) // expected-warning {{pragma
> >> include_alias requires matching include directives (e.g
> >> include_alias("foo.h", "bar.h") or include_alias(<foo.h>, <bar.h>))}}
> >> +#pragma include_alias("test.h") // expected-warning {{pragma
> >> include_alias expected ','}}
> >> +
> >> +// Make sure that the names match exactly for a replacement, including
> >> path information. If
> >> +// this were to fail, we would get a file not found error
> >> +#pragma include_alias(".\pp-record.h", "does_not_exist.h")
> >> +#include "pp-record.h"
> >
>
> Hi Richard -- I've addressed almost all of the points in your review,
> and have attached a patch for you to check over again. Thank you for
> all the helpful feedback!
A few small things, then this is good to go:
+ //// IncludeAliases - maps include file names (including the quotes or
+ /// angle brackets) to other include file names. This is used to
support the
+ /// include_alias pragma for Microsoft compatibility.
+ typedef llvm::StringMap<std::string, llvm::BumpPtrAllocator>
+ IncludeAliasMap;
+ IncludeAliasMap IncludeAliases;
This last line is too indented.
+ /// MapHeader - Maps one header file name to a different header file
name,
+ /// for use with the include_alias pragma. Note that the source file
name
+ // should include the angle brackets or quotes. Returns StringRef as
null
+ // if the header cannot be mapped.
+ std::string MapHeader(StringRef Source) {
Please return StringRef as the comment suggests here. Also, use '///' for
all four lines of the comment. (And likewise in AddHeaderMapping).
+ // Do any filename replacements before anything else
+ IncludeAliasMap::const_iterator Iter =
+ IncludeAliases.find(Source);
+ if (Iter != IncludeAliases.end())
+ return Iter->second;
+ return std::string();
+void Preprocessor::HandlePragmaIncludeAlias(Token& Tok) {
Please use ' &', not '& '.
+ unsigned int diag;
Please capitalize this (and rename to something like DiagID to avoid
shadowing the Diag member function).
+// Test to make sure there are no use-after-free problems
+#define B "pp-record.h"
+#pragma include_alias("quux.h", B)
Did the use-after-free actually affect this case? I'd hope that getSpelling
is able to avoid the copy here (and thus not use the stack buffer).
The only thing I didn't address was:
>
> > Could this repeated code be factored out? (I imagine we have largely the
> > same pattern in some other places too.)
>
> I'm guessing the code could be refactored out, but I'd rather that be
> a separate patch since it touches on more than just include_alias.
>
That's fine.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120301/280ddcb8/attachment.html>
More information about the cfe-commits
mailing list