[SubZero] Generate MIPS.abiflags section

[SubZero] Generate MIPS.abiflags section

The patch generates MIPS.abiflags section. This section contains a
versioned data structure with essential information required for
loader to determine the requirements of the application.

R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=3da9f6571608b337af7c9d2ad9e0ca938bb49ae7

[jaydeep.patil, 2016-11-03 06:35:33]

Description was changed from

==========
[SubZero] Generate MIPS.abiflags section

The patch generates MIPS.abiflags section. This section contains a
versioned data structure with essential information required for
loader to determine the requirements of the application.
==========

to

==========
[SubZero] Generate MIPS.abiflags section

The patch generates MIPS.abiflags section. This section contains a
versioned data structure with essential information required for
loader to determine the requirements of the application.
==========

[jaydeep.patil, 2016-11-03 06:35:33]

jaydeep.patil@imgtec.com changed reviewers:
+ eholk@chromium.org, jpp@chromium.org, kschimpf@google.com,
makdstefan@gmail.com, sagar.thakur@imgtec.com, stichnot@chromium.org

[Jim Stichnoth, 2016-11-03 13:23:40]

https://codereview.chromium.org/2471883005/diff/1/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/2471883005/diff/1/src/IceTargetLowering.h#new...
src/IceTargetLowering.h:619: virtual void emitTargetRODataSections() { return; }
This doesn't need the "return;".  Just:

  virtual void emitTargetRODataSections() {}

https://codereview.chromium.org/2471883005/diff/1/src/IceTargetLoweringMIPS32.h
File src/IceTargetLoweringMIPS32.h (right):

https://codereview.chromium.org/2471883005/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.h:894: 
Most of this looks like it was taken from
llvm/lib/Target/Mips/MCTargetDesc/MipsABIFlagsSection.h .  I don't think it's
appropriate to #include that file directly, but maybe a comment mentioning that?
 Or if they both came from a common source, document that?

https://codereview.chromium.org/2471883005/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.h:974: uint16_t Version;
I think you'd be better off doing something like this:
  uint16_t Version = AFL_VERSION_V0;
  uint8_t ISALevel = AFL_ISA_LEVEL_MIPS32;
  ...

  MipsABIFlagsSection() = default;

and then you don't need the setAbiFlags() method at all.

https://codereview.chromium.org/2471883005/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.h:986: public:
"public:" isn't needed since this is a struct not a class.

https://codereview.chromium.org/2471883005/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.h:987: void setAbiFlags(void) {
Remove the void arg specification (not needed in C++).

[jaydeep.patil, 2016-11-04 04:34:47]

https://codereview.chromium.org/2471883005/diff/1/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/2471883005/diff/1/src/IceTargetLowering.h#new...
src/IceTargetLowering.h:619: virtual void emitTargetRODataSections() { return; }
On 2016/11/03 13:23:40, Jim Stichnoth wrote:
> This doesn't need the "return;".  Just:
> 
>   virtual void emitTargetRODataSections() {}

Done.

https://codereview.chromium.org/2471883005/diff/1/src/IceTargetLoweringMIPS32.h
File src/IceTargetLoweringMIPS32.h (right):

https://codereview.chromium.org/2471883005/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.h:894: 
On 2016/11/03 13:23:40, Jim Stichnoth wrote:
> Most of this looks like it was taken from
> llvm/lib/Target/Mips/MCTargetDesc/MipsABIFlagsSection.h .  I don't think it's
> appropriate to #include that file directly, but maybe a comment mentioning
that?
>  Or if they both came from a common source, document that?

Done.

https://codereview.chromium.org/2471883005/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.h:974: uint16_t Version;
On 2016/11/03 13:23:40, Jim Stichnoth wrote:
> I think you'd be better off doing something like this:
>   uint16_t Version = AFL_VERSION_V0;
>   uint8_t ISALevel = AFL_ISA_LEVEL_MIPS32;
>   ...
> 
>   MipsABIFlagsSection() = default;
> 
> and then you don't need the setAbiFlags() method at all.

Done.

https://codereview.chromium.org/2471883005/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.h:986: public:
On 2016/11/03 13:23:40, Jim Stichnoth wrote:
> "public:" isn't needed since this is a struct not a class.

Done.

https://codereview.chromium.org/2471883005/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.h:987: void setAbiFlags(void) {
On 2016/11/03 13:23:40, Jim Stichnoth wrote:
> Remove the void arg specification (not needed in C++).

Removed the function.

[Jim Stichnoth, 2016-11-04 05:51:08]

lgtm

[Jim Stichnoth, 2016-11-04 05:54:11]

Description was changed from

==========
[SubZero] Generate MIPS.abiflags section

The patch generates MIPS.abiflags section. This section contains a
versioned data structure with essential information required for
loader to determine the requirements of the application.
==========

to

==========
[SubZero] Generate MIPS.abiflags section

The patch generates MIPS.abiflags section. This section contains a
versioned data structure with essential information required for
loader to determine the requirements of the application.

R=stichnot@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[Jim Stichnoth, 2016-11-04 05:54:15]

Committed patchset #2 (id:20001) manually as
3da9f6571608b337af7c9d2ad9e0ca938bb49ae7 (presubmit successful).