r302966 - Remove unused tracking of owning module for MacroInfo objects.
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Thu May 18 15:58:52 PDT 2017
On 18 May 2017 at 13:28, Dehao Chen <dehao at google.com> wrote:
> My understanding is that r302938 makes clang generate incorrect code
> (clang itself), which lead to unexpected clang behavior. Is it correct?
>
Yes, I believe so, specifically when the stage2 clang is built with
-fsanitize=memory and (I think) -O2.
If yes, how can I reproduce this issue so that I can try to triage/fix the
> problem?
>
I'll provide you with instructions offline.
Thanks,
> Dehao
>
> On Thu, May 18, 2017 at 1:22 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
>
>> On 18 May 2017 1:19 pm, "Richard Smith" <richard at metafoo.co.uk> wrote:
>>
>> On 18 May 2017 1:14 pm, "Dehao Chen" <dehao at google.com> wrote:
>>
>> What's the issue? Build breaking? Performance regression? It's not clear
>> from the limit info in this thread...
>>
>>
>> r302938 introduced or exposed a miscompile that causes a stage2 msan
>> build of Clang to misbehave:
>>
>>
>> To be fair, we don't know for sure that it's a miscompile. This code
>> works with older clang and with other compilers and is clean under the
>> sanitizers, but it might still be that there's some UB here.
>>
>> http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-boo
>> tstrap/builds/1349/steps/check-clang%20msan/logs/stdio
>>
>> From my post to another branch of this thread:
>>
>> I grabbed a clang binary from the build bot and have been trying to
>> figure out what's gone wrong. So far it looks like the msan-enabled stage1
>> miscompiled some part of clang's lib/Lex/TokenLexer.cpp (but I'm not sure
>> of that). It *looks* like TokenLexer::ExpandFunctionArguments is
>> corrupting the Flags member of the token, somewhere around the "if
>> (!VaArgsPseudoPaste)" block. I see what looks like a Token::Flags value of
>> 0x2150, which is garbage: the highest assigned flag is 0x400.
>>
>> Dehao
>>
>> On Thu, May 18, 2017 at 1:02 PM, Vitaly Buka <vitalybuka at google.com>
>> wrote:
>>
>>> Local build: r302937 no issue, r302938 has issue.
>>>
>>> On Thu, May 18, 2017 at 7:23 AM Dehao Chen <dehao at google.com> wrote:
>>>
>>>> Could you give some context on how r302938 is related to this?
>>>>
>>>> Thanks,
>>>> Dehao
>>>>
>>>> On Wed, May 17, 2017 at 11:14 PM, Vitaly Buka <vitalybuka at google.com>
>>>> wrote:
>>>>
>>>>> +Dehao Chen <dehao at google.com>
>>>>> it started from r302938
>>>>>
>>>>> On Wed, May 17, 2017 at 8:09 PM Jordan Rose via cfe-commits <
>>>>> cfe-commits at lists.llvm.org> wrote:
>>>>>
>>>>>> Thanks, this is helpful!
>>>>>>
>>>>>>
>>>>>> On May 16, 2017, at 12:26, Richard Smith <richard at metafoo.co.uk>
>>>>>> wrote:
>>>>>>
>>>>>> On 15 May 2017 at 10:28, Jordan Rose via cfe-commits <
>>>>>> cfe-commits at lists.llvm.org> wrote:
>>>>>>
>>>>>>> Hi, Richard. Swift was using this information in order to put
>>>>>>> imported macros in a particular context. It wouldn't surprise me to hear
>>>>>>> that we were doing it wrong, and that there's a better way to go from a
>>>>>>> macro back to a module, but is there a recommended replacement?
>>>>>>>
>>>>>>
>>>>>> The recommended way to connect macros to modules is via the
>>>>>> ModuleMacro objects, which represent a macro exported from a module. You
>>>>>> can query the exported macro for a (module, identifier) pair with
>>>>>> Preprocessor::getModuleMacro, or walk the ModuleMacro graph for an
>>>>>> identifier by starting from Preprocessor::getLeafModuleMacros.
>>>>>>
>>>>>> If you alternatively want to know the set of macros that would be
>>>>>> visible with a given set of imports, after setting up that state you can
>>>>>> walk the range produced by Preprocessor::macros(true) and query
>>>>>> getActiveModuleMacros on each MacroState.
>>>>>>
>>>>>> If you want to know "what is the set of macros exported directly by
>>>>>> this module?", we don't have a prebuilt mechanism for that, since no
>>>>>> in-tree client wants that information, but one way would be to walk
>>>>>> macros(true) and query getModuleMacro(module, identifier) on each one.
>>>>>>
>>>>>> Thanks,
>>>>>>> Jordan
>>>>>>>
>>>>>>>
>>>>>>> > On May 12, 2017, at 16:40, Richard Smith via cfe-commits <
>>>>>>> cfe-commits at lists.llvm.org> wrote:
>>>>>>> >
>>>>>>> > Author: rsmith
>>>>>>> > Date: Fri May 12 18:40:52 2017
>>>>>>> > New Revision: 302966
>>>>>>> >
>>>>>>> > URL: http://llvm.org/viewvc/llvm-project?rev=302966&view=rev
>>>>>>> > Log:
>>>>>>> > Remove unused tracking of owning module for MacroInfo objects.
>>>>>>> >
>>>>>>> > Modified:
>>>>>>> > cfe/trunk/include/clang/Lex/MacroInfo.h
>>>>>>> > cfe/trunk/include/clang/Lex/Preprocessor.h
>>>>>>> > cfe/trunk/lib/Lex/MacroInfo.cpp
>>>>>>> > cfe/trunk/lib/Lex/PPDirectives.cpp
>>>>>>> > cfe/trunk/lib/Lex/Preprocessor.cpp
>>>>>>> > cfe/trunk/lib/Serialization/ASTReader.cpp
>>>>>>> > cfe/trunk/lib/Serialization/ASTWriter.cpp
>>>>>>> >
>>>>>>> > Modified: cfe/trunk/include/clang/Lex/MacroInfo.h
>>>>>>> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/
>>>>>>> Lex/MacroInfo.h?rev=302966&r1=302965&r2=302966&view=diff
>>>>>>> > ============================================================
>>>>>>> ==================
>>>>>>> > --- cfe/trunk/include/clang/Lex/MacroInfo.h (original)
>>>>>>> > +++ cfe/trunk/include/clang/Lex/MacroInfo.h Fri May 12 18:40:52
>>>>>>> 2017
>>>>>>> > @@ -105,9 +105,6 @@ class MacroInfo {
>>>>>>> > /// \brief Must warn if the macro is unused at the end of
>>>>>>> translation unit.
>>>>>>> > bool IsWarnIfUnused : 1;
>>>>>>> >
>>>>>>> > - /// \brief Whether this macro info was loaded from an AST file.
>>>>>>> > - bool FromASTFile : 1;
>>>>>>> > -
>>>>>>> > /// \brief Whether this macro was used as header guard.
>>>>>>> > bool UsedForHeaderGuard : 1;
>>>>>>> >
>>>>>>> > @@ -264,34 +261,16 @@ public:
>>>>>>> > IsDisabled = true;
>>>>>>> > }
>>>>>>> >
>>>>>>> > - /// \brief Determine whether this macro info came from an AST
>>>>>>> file (such as
>>>>>>> > - /// a precompiled header or module) rather than having been
>>>>>>> parsed.
>>>>>>> > - bool isFromASTFile() const { return FromASTFile; }
>>>>>>> > -
>>>>>>> > /// \brief Determine whether this macro was used for a header
>>>>>>> guard.
>>>>>>> > bool isUsedForHeaderGuard() const { return UsedForHeaderGuard; }
>>>>>>> >
>>>>>>> > void setUsedForHeaderGuard(bool Val) { UsedForHeaderGuard = Val;
>>>>>>> }
>>>>>>> >
>>>>>>> > - /// \brief Retrieve the global ID of the module that owns this
>>>>>>> particular
>>>>>>> > - /// macro info.
>>>>>>> > - unsigned getOwningModuleID() const {
>>>>>>> > - if (isFromASTFile())
>>>>>>> > - return *(const unsigned *)(this + 1);
>>>>>>> > -
>>>>>>> > - return 0;
>>>>>>> > - }
>>>>>>> > -
>>>>>>> > void dump() const;
>>>>>>> >
>>>>>>> > private:
>>>>>>> > unsigned getDefinitionLengthSlow(const SourceManager &SM) const;
>>>>>>> >
>>>>>>> > - void setOwningModuleID(unsigned ID) {
>>>>>>> > - assert(isFromASTFile());
>>>>>>> > - *(unsigned *)(this + 1) = ID;
>>>>>>> > - }
>>>>>>> > -
>>>>>>> > friend class Preprocessor;
>>>>>>> > };
>>>>>>> >
>>>>>>> >
>>>>>>> > Modified: cfe/trunk/include/clang/Lex/Preprocessor.h
>>>>>>> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/
>>>>>>> Lex/Preprocessor.h?rev=302966&r1=302965&r2=302966&view=diff
>>>>>>> > ============================================================
>>>>>>> ==================
>>>>>>> > --- cfe/trunk/include/clang/Lex/Preprocessor.h (original)
>>>>>>> > +++ cfe/trunk/include/clang/Lex/Preprocessor.h Fri May 12
>>>>>>> 18:40:52 2017
>>>>>>> > @@ -644,14 +644,6 @@ class Preprocessor {
>>>>>>> > /// of that list.
>>>>>>> > MacroInfoChain *MIChainHead;
>>>>>>> >
>>>>>>> > - struct DeserializedMacroInfoChain {
>>>>>>> > - MacroInfo MI;
>>>>>>> > - unsigned OwningModuleID; // MUST be immediately after the
>>>>>>> MacroInfo object
>>>>>>> > - // so it can be accessed by
>>>>>>> MacroInfo::getOwningModuleID().
>>>>>>> > - DeserializedMacroInfoChain *Next;
>>>>>>> > - };
>>>>>>> > - DeserializedMacroInfoChain *DeserialMIChainHead;
>>>>>>> > -
>>>>>>> > void updateOutOfDateIdentifier(IdentifierInfo &II) const;
>>>>>>> >
>>>>>>> > public:
>>>>>>> > @@ -1669,10 +1661,6 @@ public:
>>>>>>> > /// \brief Allocate a new MacroInfo object with the provided
>>>>>>> SourceLocation.
>>>>>>> > MacroInfo *AllocateMacroInfo(SourceLocation L);
>>>>>>> >
>>>>>>> > - /// \brief Allocate a new MacroInfo object loaded from an AST
>>>>>>> file.
>>>>>>> > - MacroInfo *AllocateDeserializedMacroInfo(SourceLocation L,
>>>>>>> > - unsigned SubModuleID);
>>>>>>> > -
>>>>>>> > /// \brief Turn the specified lexer token into a fully checked
>>>>>>> and spelled
>>>>>>> > /// filename, e.g. as an operand of \#include.
>>>>>>> > ///
>>>>>>> > @@ -1764,9 +1752,6 @@ private:
>>>>>>> > /// macro name.
>>>>>>> > void updateModuleMacroInfo(const IdentifierInfo *II,
>>>>>>> ModuleMacroInfo &Info);
>>>>>>> >
>>>>>>> > - /// \brief Allocate a new MacroInfo object.
>>>>>>> > - MacroInfo *AllocateMacroInfo();
>>>>>>> > -
>>>>>>> > DefMacroDirective *AllocateDefMacroDirective(MacroInfo *MI,
>>>>>>> > SourceLocation Loc);
>>>>>>> > UndefMacroDirective *AllocateUndefMacroDirective(SourceLocation
>>>>>>> UndefLoc);
>>>>>>> >
>>>>>>> > Modified: cfe/trunk/lib/Lex/MacroInfo.cpp
>>>>>>> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/MacroI
>>>>>>> nfo.cpp?rev=302966&r1=302965&r2=302966&view=diff
>>>>>>> > ============================================================
>>>>>>> ==================
>>>>>>> > --- cfe/trunk/lib/Lex/MacroInfo.cpp (original)
>>>>>>> > +++ cfe/trunk/lib/Lex/MacroInfo.cpp Fri May 12 18:40:52 2017
>>>>>>> > @@ -29,7 +29,6 @@ MacroInfo::MacroInfo(SourceLocation DefL
>>>>>>> > IsUsed(false),
>>>>>>> > IsAllowRedefinitionsWithoutWarning(false),
>>>>>>> > IsWarnIfUnused(false),
>>>>>>> > - FromASTFile(false),
>>>>>>> > UsedForHeaderGuard(false) {
>>>>>>> > }
>>>>>>> >
>>>>>>> > @@ -137,7 +136,6 @@ LLVM_DUMP_METHOD void MacroInfo::dump()
>>>>>>> > if (IsAllowRedefinitionsWithoutWarning)
>>>>>>> > Out << " allow_redefinitions_without_warning";
>>>>>>> > if (IsWarnIfUnused) Out << " warn_if_unused";
>>>>>>> > - if (FromASTFile) Out << " imported";
>>>>>>> > if (UsedForHeaderGuard) Out << " header_guard";
>>>>>>> >
>>>>>>> > Out << "\n #define <macro>";
>>>>>>> >
>>>>>>> > Modified: cfe/trunk/lib/Lex/PPDirectives.cpp
>>>>>>> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PPDire
>>>>>>> ctives.cpp?rev=302966&r1=302965&r2=302966&view=diff
>>>>>>> > ============================================================
>>>>>>> ==================
>>>>>>> > --- cfe/trunk/lib/Lex/PPDirectives.cpp (original)
>>>>>>> > +++ cfe/trunk/lib/Lex/PPDirectives.cpp Fri May 12 18:40:52 2017
>>>>>>> > @@ -54,35 +54,12 @@ using namespace clang;
>>>>>>> > // Utility Methods for Preprocessor Directive Handling.
>>>>>>> > //===-------------------------------------------------------
>>>>>>> ---------------===//
>>>>>>> >
>>>>>>> > -MacroInfo *Preprocessor::AllocateMacroInfo() {
>>>>>>> > - MacroInfoChain *MIChain = BP.Allocate<MacroInfoChain>();
>>>>>>> > - MIChain->Next = MIChainHead;
>>>>>>> > +MacroInfo *Preprocessor::AllocateMacroInfo(SourceLocation L) {
>>>>>>> > + auto *MIChain = new (BP) MacroInfoChain{L, MIChainHead};
>>>>>>> > MIChainHead = MIChain;
>>>>>>> > return &MIChain->MI;
>>>>>>> > }
>>>>>>> >
>>>>>>> > -MacroInfo *Preprocessor::AllocateMacroInfo(SourceLocation L) {
>>>>>>> > - MacroInfo *MI = AllocateMacroInfo();
>>>>>>> > - new (MI) MacroInfo(L);
>>>>>>> > - return MI;
>>>>>>> > -}
>>>>>>> > -
>>>>>>> > -MacroInfo *Preprocessor::AllocateDeserializedMacroInfo(SourceLocation
>>>>>>> L,
>>>>>>> > - unsigned
>>>>>>> SubModuleID) {
>>>>>>> > - static_assert(alignof(MacroInfo) >= sizeof(SubModuleID),
>>>>>>> > - "alignment for MacroInfo is less than the ID");
>>>>>>> > - DeserializedMacroInfoChain *MIChain =
>>>>>>> > - BP.Allocate<DeserializedMacroInfoChain>();
>>>>>>> > - MIChain->Next = DeserialMIChainHead;
>>>>>>> > - DeserialMIChainHead = MIChain;
>>>>>>> > -
>>>>>>> > - MacroInfo *MI = &MIChain->MI;
>>>>>>> > - new (MI) MacroInfo(L);
>>>>>>> > - MI->FromASTFile = true;
>>>>>>> > - MI->setOwningModuleID(SubModuleID);
>>>>>>> > - return MI;
>>>>>>> > -}
>>>>>>> > -
>>>>>>> > DefMacroDirective *Preprocessor::AllocateDefMacroDirective(MacroInfo
>>>>>>> *MI,
>>>>>>> >
>>>>>>> SourceLocation Loc) {
>>>>>>> > return new (BP) DefMacroDirective(MI, Loc);
>>>>>>> >
>>>>>>> > Modified: cfe/trunk/lib/Lex/Preprocessor.cpp
>>>>>>> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/Prepro
>>>>>>> cessor.cpp?rev=302966&r1=302965&r2=302966&view=diff
>>>>>>> > ============================================================
>>>>>>> ==================
>>>>>>> > --- cfe/trunk/lib/Lex/Preprocessor.cpp (original)
>>>>>>> > +++ cfe/trunk/lib/Lex/Preprocessor.cpp Fri May 12 18:40:52 2017
>>>>>>> > @@ -88,7 +88,7 @@ Preprocessor::Preprocessor(std::shared_p
>>>>>>> > CurDirLookup(nullptr), CurLexerKind(CLK_Lexer),
>>>>>>> > CurLexerSubmodule(nullptr), Callbacks(nullptr),
>>>>>>> > CurSubmoduleState(&NullSubmoduleState),
>>>>>>> MacroArgCache(nullptr),
>>>>>>> > - Record(nullptr), MIChainHead(nullptr),
>>>>>>> DeserialMIChainHead(nullptr) {
>>>>>>> > + Record(nullptr), MIChainHead(nullptr) {
>>>>>>> > OwnsHeaderSearch = OwnsHeaders;
>>>>>>> >
>>>>>>> > CounterValue = 0; // __COUNTER__ starts at 0.
>>>>>>> > @@ -169,11 +169,6 @@ Preprocessor::~Preprocessor() {
>>>>>>> > std::fill(TokenLexerCache, TokenLexerCache +
>>>>>>> NumCachedTokenLexers, nullptr);
>>>>>>> > CurTokenLexer.reset();
>>>>>>> >
>>>>>>> > - while (DeserializedMacroInfoChain *I = DeserialMIChainHead) {
>>>>>>> > - DeserialMIChainHead = I->Next;
>>>>>>> > - I->~DeserializedMacroInfoChain();
>>>>>>> > - }
>>>>>>> > -
>>>>>>> > // Free any cached MacroArgs.
>>>>>>> > for (MacroArgs *ArgList = MacroArgCache; ArgList;)
>>>>>>> > ArgList = ArgList->deallocate();
>>>>>>> >
>>>>>>> > Modified: cfe/trunk/lib/Serialization/ASTReader.cpp
>>>>>>> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serializat
>>>>>>> ion/ASTReader.cpp?rev=302966&r1=302965&r2=302966&view=diff
>>>>>>> > ============================================================
>>>>>>> ==================
>>>>>>> > --- cfe/trunk/lib/Serialization/ASTReader.cpp (original)
>>>>>>> > +++ cfe/trunk/lib/Serialization/ASTReader.cpp Fri May 12 18:40:52
>>>>>>> 2017
>>>>>>> > @@ -1534,9 +1534,8 @@ MacroInfo *ASTReader::ReadMacroRecord(Mo
>>>>>>> > return Macro;
>>>>>>> >
>>>>>>> > unsigned NextIndex = 1; // Skip identifier ID.
>>>>>>> > - SubmoduleID SubModID = getGlobalSubmoduleID(F,
>>>>>>> Record[NextIndex++]);
>>>>>>> > SourceLocation Loc = ReadSourceLocation(F, Record,
>>>>>>> NextIndex);
>>>>>>> > - MacroInfo *MI = PP.AllocateDeserializedMacroInfo(Loc,
>>>>>>> SubModID);
>>>>>>> > + MacroInfo *MI = PP.AllocateMacroInfo(Loc);
>>>>>>> > MI->setDefinitionEndLoc(ReadSourceLocation(F, Record,
>>>>>>> NextIndex));
>>>>>>> > MI->setIsUsed(Record[NextIndex++]);
>>>>>>> > MI->setUsedForHeaderGuard(Record[NextIndex++]);
>>>>>>> >
>>>>>>> > Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
>>>>>>> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serializat
>>>>>>> ion/ASTWriter.cpp?rev=302966&r1=302965&r2=302966&view=diff
>>>>>>> > ============================================================
>>>>>>> ==================
>>>>>>> > --- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
>>>>>>> > +++ cfe/trunk/lib/Serialization/ASTWriter.cpp Fri May 12 18:40:52
>>>>>>> 2017
>>>>>>> > @@ -2413,7 +2413,6 @@ void ASTWriter::WritePreprocessor(const
>>>>>>> > }
>>>>>>> >
>>>>>>> > AddIdentifierRef(Name, Record);
>>>>>>> > - Record.push_back(inferSubmoduleIDFromLocation(MI->getDefinit
>>>>>>> ionLoc()));
>>>>>>> > AddSourceLocation(MI->getDefinitionLoc(), Record);
>>>>>>> > AddSourceLocation(MI->getDefinitionEndLoc(), Record);
>>>>>>> > Record.push_back(MI->isUsed());
>>>>>>> >
>>>>>>> >
>>>>>>> > _______________________________________________
>>>>>>> > cfe-commits mailing list
>>>>>>> > cfe-commits at lists.llvm.org
>>>>>>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> cfe-commits mailing list
>>>>>>> cfe-commits at lists.llvm.org
>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> cfe-commits mailing list
>>>>>> cfe-commits at lists.llvm.org
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>>
>>>>>
>>>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170518/55a253cc/attachment-0001.html>
More information about the cfe-commits
mailing list