[llvm] r287839 - Rely on a single DWARF version instead of having two copies
Greg Clayton via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 13 11:02:30 PST 2016
Thanks for looking at this. I will check if a fix that uses the AsmPrinter helper function to set the DWARF version.
Greg
> On Dec 13, 2016, at 10:45 AM, Robinson, Paul <paul.robinson at sony.com> wrote:
>
>
>
>> -----Original Message-----
>> From: Greg Clayton [mailto:gclayton at apple.com]
>> Sent: Tuesday, December 13, 2016 9:04 AM
>> To: Robinson, Paul
>> Cc: llvm-commits at lists.llvm.org
>> Subject: Re: [llvm] r287839 - Rely on a single DWARF version instead of
>> having two copies
>>
>>
>>> On Nov 28, 2016, at 10:17 AM, Robinson, Paul <paul.robinson at sony.com>
>> wrote:
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On
>> Behalf
>>>> Of Greg Clayton via llvm-commits
>>>> Sent: Wednesday, November 23, 2016 3:31 PM
>>>> To: llvm-commits at lists.llvm.org
>>>> Subject: [llvm] r287839 - Rely on a single DWARF version instead of
>> having
>>>> two copies
>>>>
>>>> Author: gclayton
>>>> Date: Wed Nov 23 17:30:37 2016
>>>> New Revision: 287839
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=287839&view=rev
>>>> Log:
>>>> Rely on a single DWARF version instead of having two copies
>>>>
>>>> This patch makes AsmPrinter less reliant on DwarfDebug by relying on
>> the
>>>> DWARF version in the AsmPrinter's MCStreamer's MCContext. This allows
>> us
>>>> to remove the redundant DWARF version from DwarfDebug. It also lets us
>>>> change code that used to access the AsmPrinter's DwarfDebug just to get
>> to
>>>> the DWARF version by changing the DWARF version accessor on AsmPrinter
>> so
>>>> that it grabs the version from its MCStreamer's MCContext.
>>>>
>>>> Differential Revision: https://reviews.llvm.org/D27032
>>>>
>>>> Modified:
>>>> llvm/trunk/include/llvm/CodeGen/AsmPrinter.h
>>>> llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
>>>> llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
>>>> llvm/trunk/lib/CodeGen/AsmPrinter/DIE.cpp
>>>> llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>>> llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h
>>>> llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
>>>>
>>>> Modified: llvm/trunk/include/llvm/CodeGen/AsmPrinter.h
>>>> URL: http://llvm.org/viewvc/llvm-
>>>>
>> project/llvm/trunk/include/llvm/CodeGen/AsmPrinter.h?rev=287839&r1=287838&
>>>> r2=287839&view=diff
>>>>
>> ==========================================================================
>>>> ====
>>>> --- llvm/trunk/include/llvm/CodeGen/AsmPrinter.h (original)
>>>> +++ llvm/trunk/include/llvm/CodeGen/AsmPrinter.h Wed Nov 23 17:30:37
>> 2016
>>>> @@ -149,6 +149,9 @@ public:
>>>> DwarfDebug *getDwarfDebug() { return DD; }
>>>> DwarfDebug *getDwarfDebug() const { return DD; }
>>>>
>>>> + uint16_t getDwarfVersion() const;
>>>> + void setDwarfVersion(uint16_t Version);
>>>> +
>>>> bool isPositionIndependent() const;
>>>>
>>>> /// Return true if assembly output should contain comments.
>>>>
>>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
>>>> URL: http://llvm.org/viewvc/llvm-
>>>>
>> project/llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinter.cpp?rev=287839&r1=287
>>>> 838&r2=287839&view=diff
>>>>
>> ==========================================================================
>>>> ====
>>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinter.cpp (original)
>>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinter.cpp Wed Nov 23
>> 17:30:37
>>>> 2016
>>>> @@ -2620,3 +2620,11 @@ void AsmPrinter::recordSled(MCSymbol *Sl
>>>> Sleds.emplace_back(
>>>> XRayFunctionEntry{ Sled, CurrentFnSym, Kind, AlwaysInstrument, Fn
>> });
>>>> }
>>>> +
>>>> +uint16_t AsmPrinter::getDwarfVersion() const {
>>>> + return OutStreamer->getContext().getDwarfVersion();
>>>> +}
>>>> +
>>>> +void AsmPrinter::setDwarfVersion(uint16_t Version) {
>>>> + OutStreamer->getContext().setDwarfVersion(Version);
>>>> +}
>>>>
>>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
>>>> URL: http://llvm.org/viewvc/llvm-
>>>>
>> project/llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp?rev=287839&r
>>>> 1=287838&r2=287839&view=diff
>>>>
>> ==========================================================================
>>>> ====
>>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp (original)
>>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp Wed Nov 23
>>>> 17:30:37 2016
>>>> @@ -177,7 +177,7 @@ void AsmPrinter::emitDwarfStringOffset(D
>>>> /// EmitDwarfRegOp - Emit dwarf register operation.
>>>> void AsmPrinter::EmitDwarfRegOp(ByteStreamer &Streamer,
>>>> const MachineLocation &MLoc) const {
>>>> - DebugLocDwarfExpression Expr(getDwarfDebug()->getDwarfVersion(),
>>>> Streamer);
>>>> + DebugLocDwarfExpression Expr(getDwarfVersion(), Streamer);
>>>> const MCRegisterInfo *MRI = MMI->getContext().getRegisterInfo();
>>>> int Reg = MRI->getDwarfRegNum(MLoc.getReg(), false);
>>>> if (Reg < 0) {
>>>>
>>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DIE.cpp
>>>> URL: http://llvm.org/viewvc/llvm-
>>>>
>> project/llvm/trunk/lib/CodeGen/AsmPrinter/DIE.cpp?rev=287839&r1=287838&r2=
>>>> 287839&view=diff
>>>>
>> ==========================================================================
>>>> ====
>>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DIE.cpp (original)
>>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DIE.cpp Wed Nov 23 17:30:37 2016
>>>> @@ -298,7 +298,7 @@ unsigned DIEInteger::SizeOf(const AsmPri
>>>> case dwarf::DW_FORM_addr:
>>>> return AP->getPointerSize();
>>>> case dwarf::DW_FORM_ref_addr:
>>>> - if (AP->OutStreamer->getContext().getDwarfVersion() == 2)
>>>> + if (AP->getDwarfVersion() == 2)
>>>> return AP->getPointerSize();
>>>> return sizeof(int32_t);
>>>> default: llvm_unreachable("DIE Value form not supported yet");
>>>> @@ -466,9 +466,7 @@ unsigned DIEEntry::getRefAddrSize(const
>>>> // specified to be four bytes in the DWARF 32-bit format and eight
>>>> bytes
>>>> // in the DWARF 64-bit format, while DWARF Version 2 specifies that
>>>> such
>>>> // references have the same size as an address on the target system.
>>>> - const DwarfDebug *DD = AP->getDwarfDebug();
>>>> - assert(DD && "Expected Dwarf Debug info to be available");
>>>> - if (DD->getDwarfVersion() == 2)
>>>> + if (AP->getDwarfVersion() == 2)
>>>> return AP->getPointerSize();
>>>> return sizeof(int32_t);
>>>> }
>>>>
>>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>>> URL: http://llvm.org/viewvc/llvm-
>>>>
>> project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=287839&r1=287
>>>> 838&r2=287839&view=diff
>>>>
>> ==========================================================================
>>>> ====
>>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)
>>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Wed Nov 23
>> 17:30:37
>>>> 2016
>>>> @@ -256,7 +256,7 @@ DwarfDebug::DwarfDebug(AsmPrinter *A, Mo
>>>> UseAllLinkageNames = DwarfLinkageNames == AllLinkageNames;
>>>>
>>>> unsigned DwarfVersionNumber = Asm->TM.Options.MCOptions.DwarfVersion;
>>>> - DwarfVersion = DwarfVersionNumber ? DwarfVersionNumber
>>>> + unsigned DwarfVersion = DwarfVersionNumber ? DwarfVersionNumber
>>>> : MMI->getModule()-
>>>>> getDwarfVersion();
>>>> // Use dwarf 4 by default if nothing is requested.
>>>> DwarfVersion = DwarfVersion ? DwarfVersion : dwarf::DWARF_VERSION;
>>>
>>> This all looks funny, now that DwarfDebug is not caching its own notion
>> of the
>>> DWARF version. At the time this constructor runs, is Asm-
>>> getDwarfVersion()
>>> not going to return a consistently useful answer?
>>> --paulr
>>>
>>
>> I don't see a call to Asm->getDwarfVersion() in the above code. Are you
>> saying we should try to get the DWARF version from there? I didn't change
>> anything in how this code behaved other than to port the code that was
>> already there and simply store the result in one location in the Asm
>> streamer in Asm->OutStreamer->getContext(). This DwarfDebug class seems to
>> be initializing itself based on the user input options it got from the
>> command line and it seems to want to tell the Asm->OutStreamer-
>>> getContext() what version it should be using. Let me know if I am missing
>> your point?
>
> Okay, I needed to look at more context. I see now that the last line of
> the DwarfDebug constructor is setting Asm's notion of the DWARF version,
> which for some reason I thought was happening somewhere else--my mistake.
> Although the place that is handing DwarfVersion to Asm could use your new
> helper function. That is, it's
>
> Asm->OutStreamer->getContext().setDwarfVersion(DwarfVersion);
>
> and could be
>
> Asm->setDwarfVersion(DwarfVersion);
>
> With that, I'm good, thanks!
> --paulr
>
>>
>>>
>>>> @@ -1197,7 +1197,7 @@ void DwarfDebug::recordSourceLine(unsign
>>>> Fn = Scope->getFilename();
>>>> Dir = Scope->getDirectory();
>>>> if (auto *LBF = dyn_cast<DILexicalBlockFile>(Scope))
>>>> - if (DwarfVersion >= 4)
>>>> + if (getDwarfVersion() >= 4)
>>>> Discriminator = LBF->getDiscriminator();
>>>>
>>>> unsigned CUID = Asm->OutStreamer-
>>>>> getContext().getDwarfCompileUnitID();
>>>> @@ -1415,8 +1415,7 @@ static void emitDebugLocValue(const AsmP
>>>> const DebugLocEntry::Value &Value,
>>>> unsigned PieceOffsetInBits) {
>>>> DIExpressionCursor ExprCursor(Value.getExpression());
>>>> - DebugLocDwarfExpression DwarfExpr(AP.getDwarfDebug()-
>>>>> getDwarfVersion(),
>>>> - Streamer);
>>>> + DebugLocDwarfExpression DwarfExpr(AP.getDwarfVersion(), Streamer);
>>>> // Regular entry.
>>>> if (Value.isInt()) {
>>>> if (BT && (BT->getEncoding() == dwarf::DW_ATE_signed ||
>>>> @@ -1467,8 +1466,7 @@ void DebugLocEntry::finalize(const AsmPr
>>>> assert(Offset <= PieceOffset && "overlapping or duplicate
>> pieces");
>>>> if (Offset < PieceOffset) {
>>>> // The DWARF spec seriously mandates pieces with no locations
>> for
>>>> gaps.
>>>> - DebugLocDwarfExpression Expr(AP.getDwarfDebug()-
>>>>> getDwarfVersion(),
>>>> - Streamer);
>>>> + DebugLocDwarfExpression Expr(AP.getDwarfVersion(), Streamer);
>>>> Expr.AddOpPiece(PieceOffset-Offset, 0);
>>>> Offset += PieceOffset-Offset;
>>>> }
>>>> @@ -1983,3 +1981,7 @@ void DwarfDebug::addAccelType(StringRef
>>>> return;
>>>> AccelTypes.AddName(InfoHolder.getStringPool().getEntry(*Asm, Name),
>>>> &Die);
>>>> }
>>>> +
>>>> +uint16_t DwarfDebug::getDwarfVersion() const {
>>>> + return Asm->OutStreamer->getContext().getDwarfVersion();
>>>> +}
>>>>
>>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h
>>>> URL: http://llvm.org/viewvc/llvm-
>>>>
>> project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h?rev=287839&r1=28783
>>>> 8&r2=287839&view=diff
>>>>
>> ==========================================================================
>>>> ====
>>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h (original)
>>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h Wed Nov 23 17:30:37
>>>> 2016
>>>> @@ -254,9 +254,6 @@ class DwarfDebug : public DebugHandlerBa
>>>> /// Whether to emit all linkage names, or just abstract subprograms.
>>>> bool UseAllLinkageNames;
>>>>
>>>> - /// Version of dwarf we're emitting.
>>>> - unsigned DwarfVersion;
>>>> -
>>>> /// DWARF5 Experimental Options
>>>> /// @{
>>>> bool HasDwarfAccelTables;
>>>> @@ -515,7 +512,7 @@ public:
>>>> bool useSplitDwarf() const { return HasSplitDwarf; }
>>>>
>>>> /// Returns the Dwarf Version.
>>>> - unsigned getDwarfVersion() const { return DwarfVersion; }
>>>> + uint16_t getDwarfVersion() const;
>>>>
>>>> /// Returns the previous CU that was being updated
>>>> const DwarfCompileUnit *getPrevCU() const { return PrevCU; }
>>>>
>>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
>>>> URL: http://llvm.org/viewvc/llvm-
>>>>
>> project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp?rev=287839&r1=2878
>>>> 38&r2=287839&view=diff
>>>>
>> ==========================================================================
>>>> ====
>>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp (original)
>>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp Wed Nov 23 17:30:37
>>>> 2016
>>>> @@ -51,7 +51,7 @@ GenerateDwarfTypeUnits("generate-type-un
>>>>
>>>> DIEDwarfExpression::DIEDwarfExpression(const AsmPrinter &AP, DwarfUnit
>>>> &DU,
>>>> DIELoc &DIE)
>>>> - : DwarfExpression(AP.getDwarfDebug()->getDwarfVersion()), AP(AP),
>>>> DU(DU),
>>>> + : DwarfExpression(AP.getDwarfVersion()), AP(AP), DU(DU),
>>>> DIE(DIE) {}
>>>>
>>>> void DIEDwarfExpression::EmitOp(uint8_t Op, const char* Comment) {
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list