[cfe-commits] [PATCH] Support for pragma include_alias
Richard Smith
richard at metafoo.co.uk
Wed Feb 29 22:58:46 PST 2012
Hi,
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.
To see the use-after-free manifest, try a testcase like this:
#define B <bar>
#pragma include_alias(<foo>, B)
void f() {}
#include <foo>
On Wed, Feb 29, 2012 at 10:21 PM, 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"
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120229/fb876ae0/attachment.html>
More information about the cfe-commits
mailing list