Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(748)

Issue 15067009: LLVM: Add ELF Note section to NaCl object files identifying them as such to gold (Closed)

Created:
7 years, 7 months ago by Derek Schuff
Modified:
7 years, 7 months ago
CC:
native-client-reviews_googlegroups.com, Roland McGrath, Mark Seaborn
Base URL:
http://git.chromium.org/native_client/pnacl-llvm.git@master
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : retry upload #

Total comments: 17

Patch Set 3 : review comments and cleanup #

Patch Set 4 : #

Total comments: 18

Patch Set 5 : reviews, and add initialization to llvm-mc #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+217 lines, -35 lines) Patch
A include/llvm/MC/MCNaCl.h View 1 2 3 4 1 chunk +18 lines, -0 lines 0 comments Download
M include/llvm/Support/ELF.h View 1 2 3 4 1 chunk +7 lines, -0 lines 1 comment Download
M lib/MC/CMakeLists.txt View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A lib/MC/MCNaCl.cpp View 1 2 3 4 1 chunk +74 lines, -0 lines 0 comments Download
M lib/Target/ARM/ARMAsmPrinter.cpp View 1 2 2 chunks +10 lines, -5 lines 0 comments Download
M lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp View 1 2 3 4 1 chunk +3 lines, -7 lines 0 comments Download
M lib/Target/Mips/MCTargetDesc/MipsMCTargetDesc.cpp View 1 2 1 chunk +1 line, -10 lines 0 comments Download
M lib/Target/Mips/MipsAsmPrinter.cpp View 1 2 2 chunks +10 lines, -5 lines 0 comments Download
M lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp View 1 2 1 chunk +1 line, -8 lines 0 comments Download
M lib/Target/X86/X86AsmPrinter.cpp View 1 2 2 chunks +6 lines, -0 lines 0 comments Download
A test/MC/ARM/elf-note-nacl.ll View 1 2 3 4 1 chunk +22 lines, -0 lines 0 comments Download
A test/MC/Mips/elf-note-nacl.ll View 1 2 3 4 1 chunk +22 lines, -0 lines 0 comments Download
A test/MC/X86/elf-note-nacl.ll View 1 2 3 4 1 chunk +36 lines, -0 lines 0 comments Download
M tools/llvm-mc/llvm-mc.cpp View 1 2 3 4 2 chunks +6 lines, -0 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
Derek Schuff
7 years, 7 months ago (2013-05-09 21:38:21 UTC) #1
Mark Seaborn
https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp File lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp (right): https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp#newcode219 lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:219: const char *NoteName = ".note.NaCl.ABI.arm"; Since this is duplicated ...
7 years, 7 months ago (2013-05-09 21:54:11 UTC) #2
eliben
https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp File lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp (right): https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp#newcode219 lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:219: const char *NoteName = ".note.NaCl.ABI.arm"; Why not: const char ...
7 years, 7 months ago (2013-05-09 21:54:59 UTC) #3
Derek Schuff
PTAL. I factored this code, and the setting of bundle alignment mode all into one ...
7 years, 7 months ago (2013-05-10 18:35:30 UTC) #4
Mark Seaborn
LGTM https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp File lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp (right): https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp#newcode232 lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:232: Streamer->EmitIntValue(1, 4); // NT_VERSION On 2013/05/10 18:35:30, Derek ...
7 years, 7 months ago (2013-05-10 19:52:45 UTC) #5
Derek Schuff
https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp File lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp (right): https://codereview.chromium.org/15067009/diff/5001/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp#newcode232 lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:232: Streamer->EmitIntValue(1, 4); // NT_VERSION On 2013/05/10 19:52:45, Mark Seaborn ...
7 years, 7 months ago (2013-05-10 22:59:26 UTC) #6
Derek Schuff
Committed patchset #5 manually as r52daf9d (presubmit successful).
7 years, 7 months ago (2013-05-10 23:00:18 UTC) #7
eliben
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.h#newcode1330 include/llvm/Support/ELF.h:1330: NT_VERSION = 1 // Note contains a version string. ...
7 years, 7 months ago (2013-05-10 23:43:22 UTC) #8
Derek Schuff
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.h#newcode1330 > ...
7 years, 7 months ago (2013-05-11 00:07:15 UTC) #9
eliben
7 years, 7 months ago (2013-05-13 15:19:11 UTC) #10
Message was sent while issue was closed.
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

Powered by Google App Engine
This is Rietveld 408576698