Hi,<div><br></div><div>I see you checked this in while I was in the process of reviewing it. There are sufficient problems with the patch that I've reverted it in r151804.</div><div><br></div><div>To see the use-after-free manifest, try a testcase like this:</div>
<div><br></div><div>#define B <bar></div><div>#pragma include_alias(<foo>, B)</div><div>void f() {}</div><div>#include <foo><br><br></div><div>On Wed, Feb 29, 2012 at 10:21 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</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">Hi,<div><br></div><div><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div>> On Sun, Feb 19, 2012 at 5:31 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>> 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></div></div></blockquote><div><br></div></div><div><div>Sorry for the delay!</div><div><br></div><div><div class="gmail_quote"><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">
<div></div></blockquote></div></div></div><div>> Index: include/clang/Basic/DiagnosticLexKinds.td</div><div><div>> ===================================================================</div><div>> --- include/clang/Basic/DiagnosticLexKinds.td<span style="white-space:pre-wrap"> </span>(revision 150897)</div>
<div>> +++ include/clang/Basic/DiagnosticLexKinds.td<span style="white-space:pre-wrap"> </span>(working copy)</div><div>> @@ -292,6 +292,15 @@</div><div>> ExtWarn<"__has_warning expected option name (e.g. \"-Wundef\")">,</div>
<div>> InGroup<MalformedWarningCheck>;</div><div>> </div><div>> +def warn_pragma_include_alias_mismatch :</div><div>> + ExtWarn<"pragma include_alias requires matching include directives "</div>
<div>> + "(e.g include_alias(\"foo.h\", \"bar.h\") or "</div><div>> + "include_alias(<foo.h>, <bar.h>))">,</div><div>> + InGroup<UnknownPragmas>;</div>
<div><br></div><div>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:</div><div>
<br>
</div><div>angle-bracketed include <%0> cannot be aliased to double-quoted include "%1"</div><div>double-quoted include "%0" cannot be aliased to angle-bracketed include <%1></div><div><br>
</div><div>> +def warn_pragma_include_alias_expected :</div><div>> + ExtWarn<"pragma include_alias expected '%0'">,</div><div>> + InGroup<UnknownPragmas>;</div><div>> +</div>
<div>
> def err__Pragma_malformed : Error<</div><div>> "_Pragma takes a parenthesized string literal">;</div><div>> def err_pragma_comment_malformed : Error<</div><div>> Index: include/clang/Lex/HeaderSearch.h</div>
<div>> ===================================================================</div><div>> --- include/clang/Lex/HeaderSearch.h<span style="white-space:pre-wrap"> </span>(revision 150897)</div><div>> +++ include/clang/Lex/HeaderSearch.h<span style="white-space:pre-wrap"> </span>(working copy)</div>
<div>> @@ -154,6 +154,9 @@</div><div>> llvm::StringMap<const DirectoryEntry *, llvm::BumpPtrAllocator></div><div>> FrameworkMap;</div><div>> </div><div>> + llvm::StringMap<std::pair<StringRef, bool>, llvm::BumpPtrAllocator></div>
<div>> + IncludeAliasMap;</div><div><br></div><div>This map doesn't appear to be sufficient: the Microsoft documentation indicates that "foo" and <foo> can be aliased to different includes.</div>
<div><br></div><div>Also, the StringRef in the value type here will refer to a string whose lifetime has ended.</div>
<div><br></div><div>> +</div><div>> /// HeaderMaps - This is a mapping from FileEntry -> HeaderMap, uniquing</div><div>> /// headermaps. This vector owns the headermap.</div><div>> std::vector<std::pair<const FileEntry*, const HeaderMap*> > HeaderMaps;</div>
<div>> @@ -217,6 +220,25 @@</div><div>> SystemDirIdx++;</div><div>> }</div><div>> </div><div>> + /// AddHeaderMapping -- Map the source include name to the dest include name</div><div>> + void AddHeaderMapping(const StringRef& Source, const StringRef& Dest, </div>
<div><br></div><div>Generally, please write " &" not "& ". Also, StringRefs are usually passed by value.</div><div><br></div><div>> + bool IsAngled) {</div><div>> + IncludeAliasMap[Source] = std::make_pair(Dest, IsAngled);</div>
<div>> + }</div><div>> +</div><div>> + StringRef MapHeader(const StringRef& Source, bool isAngled) {</div><div>> + // Do any filename replacements before anything else</div><div>> + llvm::StringMap<std::pair<StringRef,bool>>::const_iterator iter = </div>
<div><br></div><div>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.</div>
<div><br></div><div>> + IncludeAliasMap.find(Source);</div><div>> + if (iter != IncludeAliasMap.end()) {</div><div>> + // If the angling matches, then we've found a replacement</div><div>> + if (iter->second.second == isAngled) {</div>
<div>> + return iter->second.first;</div><div>> + }</div><div><br></div><div>Prevailing llvm/clang style is to omit the braces around a simple single-statement body such as this.</div><div><br></div>
<div>
> + }</div><div>> + return Source;</div><div>> + }</div><div>> +</div><div>> /// \brief Set the path to the module cache.</div><div>> void setModuleCachePath(StringRef CachePath) {</div><div>
> ModuleCachePath = CachePath;</div><div>> Index: include/clang/Lex/Preprocessor.h</div><div>> ===================================================================</div><div>> --- include/clang/Lex/Preprocessor.h<span style="white-space:pre-wrap"> </span>(revision 150897)</div>
<div>> +++ include/clang/Lex/Preprocessor.h<span style="white-space:pre-wrap"> </span>(working copy)</div><div>> @@ -1262,6 +1262,7 @@</div><div>> void HandlePragmaMessage(Token &MessageTok);</div>
<div>> void HandlePragmaPushMacro(Token &Tok);</div><div>> void HandlePragmaPopMacro(Token &Tok);</div><div>> + void HandlePragmaIncludeAlias(Token &Tok);</div><div>> IdentifierInfo *ParsePragmaPushOrPopMacro(Token &Tok);</div>
<div>> </div><div>> // Return true and store the first token only if any CommentHandler</div><div>> Index: lib/Lex/PPDirectives.cpp</div><div>> ===================================================================</div>
<div>> --- lib/Lex/PPDirectives.cpp<span style="white-space:pre-wrap"> </span>(revision 150897)</div><div>> +++ lib/Lex/PPDirectives.cpp<span style="white-space:pre-wrap"> </span>(working copy)</div>
<div>> @@ -1297,6 +1297,9 @@</div><div>> PragmaARCCFCodeAuditedLoc = SourceLocation();</div><div>> }</div><div>> </div><div>> + // Map the filename</div><div>> + Filename = HeaderInfo.MapHeader(Filename, isAngled);</div>
<div>> +</div><div>> // Search include directories.</div><div>> const DirectoryLookup *CurDir;</div><div>> SmallString<1024> SearchPath;</div><div>> Index: lib/Lex/Pragma.cpp</div><div>> ===================================================================</div>
<div>> --- lib/Lex/Pragma.cpp<span style="white-space:pre-wrap"> </span>(revision 150897)</div><div>> +++ lib/Lex/Pragma.cpp<span style="white-space:pre-wrap"> </span>(working copy)</div>
<div>> @@ -663,6 +663,101 @@</div><div>> }</div><div>> }</div><div>> </div><div>> +void Preprocessor::HandlePragmaIncludeAlias(Token& Tok) {</div><div>> + // We will either get a quoted filename or a bracketed filename, and we </div>
<div>> + // have to track which we got. The first filename is the source name,</div><div>> + // and the second name is the mapped filename. If the first is quoted,</div><div>> + // the second must be as well (cannot mix and match quotes and brackets).</div>
<div>> + SourceLocation Loc = Tok.getLocation();</div><div>> +</div><div>> + // Get the open paren</div><div>> + Lex(Tok);</div><div>> + if (Tok.isNot(tok::l_paren)) {</div><div>> + Diag(Tok, diag::warn_pragma_include_alias_expected) << "(";</div>
<div>> + return;</div><div>> + }</div><div>> +</div><div>> + // We expect either a quoted string literal, or a bracketed name</div><div>> + Token SourceFilenameTok;</div><div>> + CurPPLexer->LexIncludeFilename(SourceFilenameTok);</div>
<div>> + if (SourceFilenameTok.is(tok::eod)) {</div><div>> + // The diagnostic has already been handled</div><div>> + return;</div><div>> + }</div><div>> +</div><div>> + StringRef SourceFileName;</div>
<div>> + SmallString<128> FileNameBuffer;</div><div>> + if (SourceFilenameTok.is(tok::string_literal) || </div><div>> + SourceFilenameTok.is(tok::angle_string_literal)) {</div><div>> + SourceFileName = getSpelling(SourceFilenameTok, FileNameBuffer);</div>
<div>> + } else if (SourceFilenameTok.is(tok::less)) {</div><div>> + // This could be a path instead of just a name</div><div>> + FileNameBuffer.push_back('<');</div><div>> + SourceLocation End;</div>
<div>> + if (ConcatenateIncludeName(FileNameBuffer, End))</div><div>> + return; // Diagnostic already emitted</div><div>> + SourceFileName = FileNameBuffer.str();</div><div>> + } else {</div><div>
> + Diag(Tok, diag::warn_pragma_include_alias_expected) << "include filename";</div>
<div><br></div><div>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?</div>
<div><br></div><div>Also, I don't see a test for this error.</div><div><br></div><div>> + return;</div><div>> + }</div><div>> + FileNameBuffer.clear();</div><div>> +</div><div>> + // Now we expect a comma, followed by another include name</div>
<div>> + Lex(Tok);</div><div>> + if (Tok.isNot(tok::comma)) {</div><div>> + Diag(Tok, diag::warn_pragma_include_alias_expected) << ",";</div><div>> + return;</div><div>> + }</div><div>
> +</div><div>> + Token ReplaceFilenameTok;</div><div>> + CurPPLexer->LexIncludeFilename(ReplaceFilenameTok);</div><div>> + if (ReplaceFilenameTok.is(tok::eod)) {</div><div>> + // The diagnostic has already been handled</div>
<div>> + return;</div><div>> + }</div><div>> +</div><div>> + StringRef ReplaceFileName;</div><div>> + if (ReplaceFilenameTok.is(tok::string_literal) || </div><div>> + ReplaceFilenameTok.is(tok::angle_string_literal)) {</div>
<div>> + ReplaceFileName = getSpelling(ReplaceFilenameTok, FileNameBuffer);</div><div>> + } else if (ReplaceFilenameTok.is(tok::less)) {</div><div>> + // This could be a path instead of just a name</div><div>
> + FileNameBuffer.push_back('<');</div><div>> + SourceLocation End;</div><div>> + if (ConcatenateIncludeName(FileNameBuffer, End))</div><div>> + return; // Diagnostic already emitted</div>
<div>> + ReplaceFileName = FileNameBuffer.str();</div><div>> + } else {</div><div>> + Diag(Tok, diag::warn_pragma_include_alias_expected) << "include filename";</div><div>> + return;</div>
<div>> + }</div><div><br></div><div>Could this repeated code be factored out? (I imagine we have largely the same pattern in some other places too.)</div><div><br></div><div>> +</div><div>> + // Finally, we expect the closing paren</div>
<div>> + Lex(Tok);</div><div>> + if (Tok.isNot(tok::r_paren)) {</div><div>> + Diag(Tok, diag::warn_pragma_include_alias_expected) << ")";</div>
<div>> + return;</div><div>> + }</div><div>> +</div><div>> + // Now that we have the source and target filenames, we need to make sure</div><div>> + // they're both of the same type (angled vs non-angled)</div>
<div>> + bool SourceIsAngled = </div><div>> + GetIncludeFilenameSpelling(SourceFilenameTok.getLocation(), </div><div>> + SourceFileName);</div><div>> + bool ReplaceIsAngled =</div>
<div>> + GetIncludeFilenameSpelling(ReplaceFilenameTok.getLocation(),</div><div>> + ReplaceFileName);</div><div>> + if (SourceIsAngled != ReplaceIsAngled) {</div><div>> + Diag(Loc, diag::warn_pragma_include_alias_mismatch);</div>
<div>> + return;</div><div>> + }</div><div>> +</div><div>> + // Now we can let the include handler know about this mapping</div><div>> + getHeaderSearchInfo().AddHeaderMapping(SourceFileName, ReplaceFileName, </div>
<div><br></div><div>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.</div><div><br></div>
<div>> + SourceIsAngled);</div><div>> +}</div><div>> +</div><div>> /// AddPragmaHandler - Add the specified pragma handler to the preprocessor.</div><div>> /// If 'Namespace' is non-null, then it is a token required to exist on the</div>
<div>> /// pragma line before the pragma string starts, e.g. "STDC" or "GCC".</div><div>> @@ -943,6 +1038,15 @@</div><div>> }</div><div>> };</div><div>> </div><div>> +/// PragmaIncludeAliasHandler - "#pragma include_alias("...")".</div>
<div>> +struct PragmaIncludeAliasHandler : public PragmaHandler {</div><div>> + PragmaIncludeAliasHandler() : PragmaHandler("include_alias") {}</div><div>> + virtual void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer,</div>
<div>> + Token &IncludeAliasTok) {</div><div>> + PP.HandlePragmaIncludeAlias(IncludeAliasTok);</div><div>> + }</div><div>> +};</div><div>> +</div><div>> /// PragmaMessageHandler - "#pragma message("...")".</div>
<div>> struct PragmaMessageHandler : public PragmaHandler {</div><div>> PragmaMessageHandler() : PragmaHandler("message") {}</div><div>> @@ -1095,5 +1199,6 @@</div><div>> // MS extensions.</div>
<div>> if (Features.MicrosoftExt) {</div><div>> AddPragmaHandler(new PragmaCommentHandler());</div><div>> + AddPragmaHandler(new PragmaIncludeAliasHandler());</div><div>> }</div><div>> }</div>
<div>> Index: test/Preprocessor/pragma_microsoft.c</div><div>> ===================================================================</div><div>> --- test/Preprocessor/pragma_microsoft.c<span style="white-space:pre-wrap"> </span>(revision 150897)</div>
<div>> +++ test/Preprocessor/pragma_microsoft.c<span style="white-space:pre-wrap"> </span>(working copy)</div><div>> @@ -38,3 +38,20 @@</div><div>> // this warning should go away.</div><div>> MACRO_WITH__PRAGMA // expected-warning {{expression result unused}}</div>
<div>> }</div><div>> +</div><div>> +</div><div>> +// This should include macro_arg_directive even though the include</div><div>> +// is looking for test.h This allows us to assign to "n"</div><div>
> +#pragma include_alias("test.h", "macro_arg_directive.h" )</div><div>> +#include "test.h"</div><div>> +void test( void ) {</div><div>> + n = 12;</div><div>> +}</div><div>> +</div>
<div>> +#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>))}}</div>
<div>> +#pragma include_alias("test.h") // expected-warning {{pragma include_alias expected ','}}</div><div>> +</div><div>> +// Make sure that the names match exactly for a replacement, including path information. If</div>
<div>> +// this were to fail, we would get a file not found error</div><div>> +#pragma include_alias(".\pp-record.h", "does_not_exist.h")</div><div>> +#include "pp-record.h"</div></div>
<div><br></div></div></div>
</blockquote></div><br></div>