LLVM: Add ELF Note section to NaCl object files identifying them as such to gold

LLVM: Add ELF Note section to NaCl object files identifying them as such to gold

This is needed to switch the native linker to one based on upstream binutils
2.23
R=mseaborn@chromium.org
BUG= https://code.google.com/p/nativeclient/issues/detail?id=2971
also related to bug https://code.google.com/p/nativeclient/issues/detail?id=3424

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=52daf9d

[Mark Seaborn, 2013-05-09 21:54:11]

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
File lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp (right):

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:219: const char *NoteName =
".note.NaCl.ABI.arm";
Since this is duplicated between ARM/x86 (and doesn't cover MIPS), how about
moving it to a common location so that it can be reused?  And then cover MIPS.
:-)

Can you add a comment to explain the purpose of this ELF note?

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:227: // maybe should use
pushSection and popSection but popSection will crash if
'maybe' -> 'Maybe'.  Make this a TODO?

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:230: Streamer->EmitIntValue(5,
4); // sizeof("NaCl")
Use strlen(NoteNamespace)

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:231: Streamer->EmitIntValue(4,
4); // sizeof("arm")
ditto

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:232: Streamer->EmitIntValue(1,
4); // NT_VERSION
Can you #define this in Support/ELF.h?

[eliben, 2013-05-09 21:54:59]

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
File lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp (right):

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:219: const char *NoteName =
".note.NaCl.ABI.arm";
Why not:

const char NoteName[] = "...."

And then you can actually use sizeof() in EmitIntValue...

https://codereview.chromium.org/15067009/diff/5001/lib/Target/X86/MCTargetDes...
File lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp (right):

https://codereview.chromium.org/15067009/diff/5001/lib/Target/X86/MCTargetDes...
lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:372: // @LOCALMOD-BEGIN
I wonder why the LOCALMOD is here, createELFStreamer seems to be the same
upstream

https://codereview.chromium.org/15067009/diff/5001/lib/Target/X86/MCTargetDes...
lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:375: if (TheTriple.isOSNaCl()) {
Move this to a separate function with a descriptive name.

Looking around even more, perhaps a cleaner LOCALMOD would be to have our own
createNaClELFStreamer that only handles the ELF case, and have it registered in 
  TargetRegistry::RegisterMCObjectStreamer

if the OS is NaCl. This way we don't have to touch the upstreamer
createMCStreamer at all.

[Derek Schuff, 2013-05-10 18:35:30]

PTAL. I factored this code, and the setting of bundle alignment mode all into
one common MCStreamer initialization, and moved the call to it from MCStreamer
creation to <target>AsmPrinter.cpp (which, despite its name, initializes modules
for object output as well). This fits better with the model of MC/the assembler
being a dumb interpreter of directives and the intelligence living in the things
that use it (and also is more analogous to what happens in nacl-gcc).

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
File lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp (right):

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:219: const char *NoteName =
".note.NaCl.ABI.arm";
On 2013/05/09 21:54:59, eliben wrote:
> Why not:
> 
> const char NoteName[] = "...."
> 
> And then you can actually use sizeof() in EmitIntValue...

I didn't end up with a named const char array for this after moving to a common
location, so just went with strlen everywhere

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:227: // maybe should use
pushSection and popSection but popSection will crash if
On 2013/05/09 21:54:11, Mark Seaborn wrote:
> 'maybe' -> 'Maybe'.  Make this a TODO?

Done.

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:230: Streamer->EmitIntValue(5,
4); // sizeof("NaCl")
On 2013/05/09 21:54:11, Mark Seaborn wrote:
> Use strlen(NoteNamespace)

Done.

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:231: Streamer->EmitIntValue(4,
4); // sizeof("arm")
On 2013/05/09 21:54:11, Mark Seaborn wrote:
> ditto

Done.

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:232: Streamer->EmitIntValue(1,
4); // NT_VERSION
On 2013/05/09 21:54:11, Mark Seaborn wrote:
> Can you #define this in Support/ELF.h?
I could. I'm actually not sure if this is 1 because it's NT_VERSION or if it's 1
because we are using ABI version 1. see
https://code.google.com/p/nativeclient/issues/detail?id=2971

https://codereview.chromium.org/15067009/diff/5001/lib/Target/X86/MCTargetDes...
File lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp (right):

https://codereview.chromium.org/15067009/diff/5001/lib/Target/X86/MCTargetDes...
lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:372: // @LOCALMOD-BEGIN
On 2013/05/09 21:54:59, eliben wrote:
> I wonder why the LOCALMOD is here, createELFStreamer seems to be the same
> upstream
I removed this localmod entirely in the cleanup.

https://codereview.chromium.org/15067009/diff/5001/lib/Target/X86/MCTargetDes...
lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:375: if (TheTriple.isOSNaCl()) {
On 2013/05/09 21:54:59, eliben wrote:
> Move this to a separate function with a descriptive name.
> 
> Looking around even more, perhaps a cleaner LOCALMOD would be to have our own
> createNaClELFStreamer that only handles the ELF case, and have it registered
in 
>   TargetRegistry::RegisterMCObjectStreamer
> 
> if the OS is NaCl. This way we don't have to touch the upstreamer
> createMCStreamer at all.
> 
That actually doesn't work because registration only distinguishes between
(arch) targets and not OSes.
But we now have our own init...

[Mark Seaborn, 2013-05-10 19:52:45]

LGTM

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
File lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp (right):

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:232: Streamer->EmitIntValue(1,
4); // NT_VERSION
On 2013/05/10 18:35:30, Derek Schuff wrote:
> On 2013/05/09 21:54:11, Mark Seaborn wrote:
> > Can you #define this in Support/ELF.h?
> I could. I'm actually not sure if this is 1 because it's NT_VERSION or if it's
1
> because we are using ABI version 1. see
> https://code.google.com/p/nativeclient/issues/detail?id=2971

It's NT_VERSION.  See gold/nacl.h in binutils:

            if ((elfcpp::Swap<32, big_endian>::readval(note_data + 0)
                 == sizeof "NaCl")
                && (elfcpp::Swap<32, big_endian>::readval(note_data + 4)
                    == nacl_abi_name_.size() + 1)
                && (elfcpp::Swap<32, big_endian>::readval(note_data + 8)
                    == elfcpp::NT_VERSION))

https://codereview.chromium.org/15067009/diff/14001/include/llvm/MC/MCNaCl.h
File include/llvm/MC/MCNaCl.h (right):

https://codereview.chromium.org/15067009/diff/14001/include/llvm/MC/MCNaCl.h#...
include/llvm/MC/MCNaCl.h:15: /// ELF section headers.
'NaCl ELF section headers' -> 'NaCl ELF note sections'?

https://codereview.chromium.org/15067009/diff/14001/lib/MC/MCNaCl.cpp
File lib/MC/MCNaCl.cpp (right):

https://codereview.chromium.org/15067009/diff/14001/lib/MC/MCNaCl.cpp#newcode17
lib/MC/MCNaCl.cpp:17: static const char *NoteNamespace = "NaCl";
static const char NoteNamespace[] = "NaCl";

That avoids a relocation.  Allegedly.

https://codereview.chromium.org/15067009/diff/14001/lib/MC/MCNaCl.cpp#newcode23
lib/MC/MCNaCl.cpp:23: errs() << "initializeNaClMCStreamer\n";
Remove this debugging output

https://codereview.chromium.org/15067009/diff/14001/lib/MC/MCNaCl.cpp#newcode58
lib/MC/MCNaCl.cpp:58: NoteName, ELF::SHT_NOTE, ELF::SHF_ALLOC | ELF::SHF_GROUP,
Nit: Indent by 4 rather than 6?

https://codereview.chromium.org/15067009/diff/14001/lib/MC/MCNaCl.cpp#newcode68
lib/MC/MCNaCl.cpp:68: Streamer.EmitValueToAlignment(4);
This one isn't needed, BTW.

https://codereview.chromium.org/15067009/diff/14001/lib/Target/ARM/MCTargetDe...
File lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp (right):

https://codereview.chromium.org/15067009/diff/14001/lib/Target/ARM/MCTargetDe...
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:213: NoExecStack,
TheTriple.getArch() == Triple::thumb);
Line >80 chars

https://codereview.chromium.org/15067009/diff/14001/test/MC/ARM/elf-note-nacl.ll
File test/MC/ARM/elf-note-nacl.ll (right):

https://codereview.chromium.org/15067009/diff/14001/test/MC/ARM/elf-note-nacl...
test/MC/ARM/elf-note-nacl.ll:12: ; to dump groups like readelf
Add "." at end of sentence to separate from the next line.

https://codereview.chromium.org/15067009/diff/14001/test/MC/ARM/elf-note-nacl...
test/MC/ARM/elf-note-nacl.ll:15: ; contents of the words in the note section
should be:
"The contents..." (for readability)

https://codereview.chromium.org/15067009/diff/14001/test/MC/ARM/elf-note-nacl...
test/MC/ARM/elf-note-nacl.ll:16: ; sizeof "NaCl"
Maybe indent for readability: ';   sizeof "NaCl"'

Same for the other tests...

[Derek Schuff, 2013-05-10 22:59:26]

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
File lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp (right):

https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDes...
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:232: Streamer->EmitIntValue(1,
4); // NT_VERSION
On 2013/05/10 19:52:45, Mark Seaborn wrote:
> On 2013/05/10 18:35:30, Derek Schuff wrote:
> > On 2013/05/09 21:54:11, Mark Seaborn wrote:
> > > Can you #define this in Support/ELF.h?
> > I could. I'm actually not sure if this is 1 because it's NT_VERSION or if
it's
> 1
> > because we are using ABI version 1. see
> > https://code.google.com/p/nativeclient/issues/detail?id=2971
> 
> It's NT_VERSION.  See gold/nacl.h in binutils:
> 
>             if ((elfcpp::Swap<32, big_endian>::readval(note_data + 0)
>                  == sizeof "NaCl")
>                 && (elfcpp::Swap<32, big_endian>::readval(note_data + 4)
>                     == nacl_abi_name_.size() + 1)
>                 && (elfcpp::Swap<32, big_endian>::readval(note_data + 8)
>                     == elfcpp::NT_VERSION))

Done.

https://codereview.chromium.org/15067009/diff/14001/include/llvm/MC/MCNaCl.h
File include/llvm/MC/MCNaCl.h (right):

https://codereview.chromium.org/15067009/diff/14001/include/llvm/MC/MCNaCl.h#...
include/llvm/MC/MCNaCl.h:15: /// ELF section headers.
On 2013/05/10 19:52:46, Mark Seaborn wrote:
> 'NaCl ELF section headers' -> 'NaCl ELF note sections'?

Done.

https://codereview.chromium.org/15067009/diff/14001/lib/MC/MCNaCl.cpp
File lib/MC/MCNaCl.cpp (right):

https://codereview.chromium.org/15067009/diff/14001/lib/MC/MCNaCl.cpp#newcode17
lib/MC/MCNaCl.cpp:17: static const char *NoteNamespace = "NaCl";
On 2013/05/10 19:52:46, Mark Seaborn wrote:
> static const char NoteNamespace[] = "NaCl";
> 
> That avoids a relocation.  Allegedly.

Done.

https://codereview.chromium.org/15067009/diff/14001/lib/MC/MCNaCl.cpp#newcode23
lib/MC/MCNaCl.cpp:23: errs() << "initializeNaClMCStreamer\n";
On 2013/05/10 19:52:46, Mark Seaborn wrote:
> Remove this debugging output

Done.

https://codereview.chromium.org/15067009/diff/14001/lib/MC/MCNaCl.cpp#newcode58
lib/MC/MCNaCl.cpp:58: NoteName, ELF::SHT_NOTE, ELF::SHF_ALLOC | ELF::SHF_GROUP,
On 2013/05/10 19:52:46, Mark Seaborn wrote:
> Nit: Indent by 4 rather than 6?

Done.

https://codereview.chromium.org/15067009/diff/14001/lib/MC/MCNaCl.cpp#newcode68
lib/MC/MCNaCl.cpp:68: Streamer.EmitValueToAlignment(4);
On 2013/05/10 19:52:46, Mark Seaborn wrote:
> This one isn't needed, BTW.

Done.

https://codereview.chromium.org/15067009/diff/14001/lib/Target/ARM/MCTargetDe...
File lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp (right):

https://codereview.chromium.org/15067009/diff/14001/lib/Target/ARM/MCTargetDe...
lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:213: NoExecStack,
TheTriple.getArch() == Triple::thumb);
On 2013/05/10 19:52:46, Mark Seaborn wrote:
> Line >80 chars

Done.

https://codereview.chromium.org/15067009/diff/14001/test/MC/ARM/elf-note-nacl.ll
File test/MC/ARM/elf-note-nacl.ll (right):

https://codereview.chromium.org/15067009/diff/14001/test/MC/ARM/elf-note-nacl...
test/MC/ARM/elf-note-nacl.ll:12: ; to dump groups like readelf
On 2013/05/10 19:52:46, Mark Seaborn wrote:
> Add "." at end of sentence to separate from the next line.

Done.

https://codereview.chromium.org/15067009/diff/14001/test/MC/ARM/elf-note-nacl...
test/MC/ARM/elf-note-nacl.ll:15: ; contents of the words in the note section
should be:
On 2013/05/10 19:52:46, Mark Seaborn wrote:
> "The contents..." (for readability)

Done.

https://codereview.chromium.org/15067009/diff/14001/test/MC/ARM/elf-note-nacl...
test/MC/ARM/elf-note-nacl.ll:16: ; sizeof "NaCl"
On 2013/05/10 19:52:46, Mark Seaborn wrote:
> Maybe indent for readability: ';   sizeof "NaCl"'
> 
> Same for the other tests...

Done.

[Derek Schuff, 2013-05-10 23:00:18]

Committed patchset #5 manually as r52daf9d (presubmit successful).

[eliben, 2013-05-10 23:43:22]

https://codereview.chromium.org/15067009/diff/20001/include/llvm/Support/ELF.h
File include/llvm/Support/ELF.h (right):

https://codereview.chromium.org/15067009/diff/20001/include/llvm/Support/ELF....
include/llvm/Support/ELF.h:1330: NT_VERSION  = 1          // Note contains a
version string.
Is this upstreamable?

[Derek Schuff, 2013-05-11 00:07:15]

On 2013/05/10 23:43:22, eliben wrote:
> https://codereview.chromium.org/15067009/diff/20001/include/llvm/Support/ELF.h
> File include/llvm/Support/ELF.h (right):
> 
>
https://codereview.chromium.org/15067009/diff/20001/include/llvm/Support/ELF....
> include/llvm/Support/ELF.h:1330: NT_VERSION  = 1          // Note contains a
> version string.
> Is this upstreamable?

Probably. I don't know if Support/ELF.h is supposed to be an
"as-complete-as-possible" mirror of GNU elf.h (or its cpp equivalent) or if it's
just supposed to be enough for what's used in LLVM, but yeah. Probably the rest
of this maybe should be upstreamed too, since it's not far from what we've
upstreamed alraedy. I just wanted to get it in here so we can get build-ids and
crash dumping for QO ASAP.

[eliben, 2013-05-13 15:19:11]

On 2013/05/11 00:07:15, Derek Schuff wrote:
> On 2013/05/10 23:43:22, eliben wrote:
> >
https://codereview.chromium.org/15067009/diff/20001/include/llvm/Support/ELF.h
> > File include/llvm/Support/ELF.h (right):
> > 
> >
>
https://codereview.chromium.org/15067009/diff/20001/include/llvm/Support/ELF....
> > include/llvm/Support/ELF.h:1330: NT_VERSION  = 1          // Note contains a
> > version string.
> > Is this upstreamable?
> 
> Probably. I don't know if Support/ELF.h is supposed to be an
> "as-complete-as-possible" mirror of GNU elf.h (or its cpp equivalent) or if
it's
> just supposed to be enough for what's used in LLVM, but yeah. Probably the
rest
> of this maybe should be upstreamed too, since it's not far from what we've
> upstreamed alraedy. I just wanted to get it in here so we can get build-ids
and
> crash dumping for QO ASAP.

Yeah, it looks pretty generic to me and it would be nice to upstream at some
point so we can avoid this being another LOCALMOD.

Eli