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