[llvm] r206653 - Update the fragments of symbols in compressed sections.

[David Blaikie]

On Fri, Apr 18, 2014 at 5:49 PM, Nick Lewycky <nlewycky at google.com> wrote:
> On 18 April 2014 14:24, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> Author: dblaikie
>> Date: Fri Apr 18 16:24:12 2014
>> New Revision: 206653
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=206653&view=rev
>> Log:
>> Update the fragments of symbols in compressed sections.
>>
>> While unnamed relocations are already cached in side tables in
>> ELFObjectWriter::RecordRelocation, symbols still need their fragments
>> updated to refer to the newly compressed fragment (even if that fragment
>> isn't big enough to fit the offset). Even though we only create
>> temporary symbols in debug info sections this comes up in 32 bit builds
>> where even temporary symbols in mergeable sections (such as debug_str)
>> have to be emitted as named symbols.
>>
>> I tried a few other ways to do this but they all didn't work for various
>> reasons:
>>
>> 1) Canonicalize the MCSymbolData in RecordRelocation, nulling out the
>> Fragment (so it didn't have to be updated by CompressDebugSection). This
>> doesn't work because some code relies on symbols having fragments to
>> indicate that they're defined, I think.
>>
>> 2) Canonicalize the MCSymbolData in RecordRelocation to be "first
>> fragment + absolute offset" so it would be cheaper to just test and
>> update the fragment in CompressDebugSections. This doesn't work because
>> the offset computed in RecordRelocation isn't that of the symbol's
>> fragment, it's the passed in fragment (I haven't figured out what that
>> fragment is - perhaps it's the location where the relocation is to be
>> written). And if the fragment offset has to be computed only for this
>> use we might as well just do it when we need to, in
>> CompressDebugSection.
>>
>> I also added an assert to help catch this a bit more clearly, even
>> though it is UB. The test case improvements would either assert fail
>> and/or valgrind vail without the fix, even if they wouldn't necessarily
>> fail the FileCheck output.
>>
>> Modified:
>>     llvm/trunk/lib/MC/ELFObjectWriter.cpp
>>     llvm/trunk/test/MC/ELF/compression.s
>>
>> Modified: llvm/trunk/lib/MC/ELFObjectWriter.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/ELFObjectWriter.cpp?rev=206653&r1=206652&r2=206653&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/MC/ELFObjectWriter.cpp (original)
>> +++ llvm/trunk/lib/MC/ELFObjectWriter.cpp Fri Apr 18 16:24:12 2014
>> @@ -616,6 +616,10 @@ static const MCSymbol *getBaseSymbol(con
>>  void ELFObjectWriter::WriteSymbol(SymbolTableWriter &Writer,
>> ELFSymbolData &MSD,
>>                                    const MCAsmLayout &Layout) {
>>    MCSymbolData &OrigData = *MSD.SymbolData;
>> +  assert(!OrigData.getFragment() ||
>> +         (&OrigData.getFragment()->getParent()->getSection() ==
>> +          &OrigData.getSymbol().getSection()) &&
>> +             "The symbol's section doesn't match the fragment's symbol");
>
>
> GCC says:
>
> lib/MC/ELFObjectWriter.cpp: In member function 'void
> {anonymous}::ELFObjectWriter::WriteSymbol({anonymous}::SymbolTableWriter&,
> {anonymous}::ELFObjectWriter::ELFSymbolData&, const llvm::MCAsmLayout&)':
> lib/MC/ELFObjectWriter.cpp:621:47: error: suggest parentheses around '&&'
> within '||' [-Werror=parentheses]

Hmm sorry, hadn't seen the bots complain about that - but perhaps it
got lost in teh noise of other buildbreaks.

Added parens in r206678.

> That sounds reasonable to me. You've got assert(!x || (y && z) &&
> "string!");

I assume Clang's warning doesn't flag this because it doesn't actually
change the semantics of the assert, which seems fair. Though adding
the parens did make the formatting a bit nicer.