On Thu, Mar 1, 2012 at 10:43 AM, Aaron Ballman <[email protected]>wrote:
> On Thu, Mar 1, 2012 at 12:21 AM, Richard Smith <[email protected]> > wrote: > > Hi, > > > >> > On Sun, Feb 19, 2012 at 5:31 PM, Aaron Ballman < > [email protected]> > >> > 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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
