- Add functionality for parsing AIX XCOFF object files header .
- Only support 32bits AIX XCOFF object files in the patch
- Print out the AIX XCOFF object file header as YAML format.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
llvm/include/llvm/Object/XCOFFObjectFile.h | ||
---|---|---|
44 ↗ | (On Diff #190835) | When _XCOFF_UOFFSET is set, the AIX headers treat this field as unsigned. Indications are that values greater than those of the corresponding signed type are indeed valid (despite difficulties in using files with such values with the AIX system linker). |
llvm/lib/Object/XCOFFObjectFile.cpp | ||
241 ↗ | (On Diff #190835) | getObject below does size checking too. What does having this check here add? |
I've added a few reviewers who have committed to the other file formats recently since none of us have a lot of experience with this part of llvm.
llvm/include/llvm/Object/XCOFFObjectFile.h | ||
---|---|---|
36 ↗ | (On Diff #190835) | Rather then defining these in the llvm::object namespace why not drop the XCOFF part of the name and make the defintion nested inside XCOFFObjectFile? |
53 ↗ | (On Diff #190835) | The only use of this is in XCOFFObjectFile.cpp, it should be defined there instead of in the header. |
The skeleton of the dumper seems fine.
My main question would be, how is this going to fit into MC and the rest of LLVM? We support 4(?) object file formats there now (ELF, COFF (pretty much assumes Windows), wasm, and MachO), and MC is not designed to be very extensible. Will Triple::isOSBinFormatCOFF return true for XCOFF, or will it be distinct?
llvm/include/llvm/Object/XCOFFObjectFile.h | ||
---|---|---|
36 ↗ | (On Diff #190835) | (Not to bikeshed too much...) Personally, I'd leave this as is. It's consistent with the other object formats. It's useful to be able to do using namespace llvm::object;, but you cannot shorten XCOFFObjectFile::Header without a typedef or type alias. |
Our intention is for XCOFF to be a distinct object file format. We had a look at the Windows COFF format and it does share some similarities with XCOFF, but overall I think they are too different to try to reconcile into a single COFF object file format. Jason already landed a patch for adding Aix and XCOFF to the triple (here) and we treat XCOFF as a distinct object format there as well.
changed the code based on the comment.
- changed the support::ubig32_t SymbolTableOffset;
- move the getObject function to llvm/lib/Object/XCOFFObjectFile.cpp
- deleted the checkSize function definition and removed the checkSize from XCOFFObjectFile::XCOFFObjectFile
llvm/include/llvm/Object/XCOFFObjectFile.h | ||
---|---|---|
36 ↗ | (On Diff #190835) | I am agreed with Reid Kleckner. |
44 ↗ | (On Diff #190835) | changed to support::ubig32_t SymbolTableOffset |
53 ↗ | (On Diff #190835) | OK. I can move it to XCOFFObjectFile.cpp |
llvm/lib/Object/XCOFFObjectFile.cpp | ||
241 ↗ | (On Diff #190835) | I have deleted the checkSize function definition and removed the check size here |
changed the code based on the comment.
1.changed the support::ubig32_t SymbolTableOffset;
2.move the getObject function to llvm/lib/Object/XCOFFObjectFile.cpp
3.deleted the checkSize function definition and removed the checkSize from XCOFFObjectFile::XCOFFObjectFile
Other the a few minor nit picks, this LGTM.
llvm/include/llvm/Object/XCOFFObjectFile.h | ||
---|---|---|
36 ↗ | (On Diff #190835) | Fair enough. |
108 ↗ | (On Diff #192273) | minor nit: add a blank line before the endif |
llvm/include/llvm/ObjectYAML/XCOFFYAML.h | ||
12 ↗ | (On Diff #192273) | blank line before the ifndef. |
19 ↗ | (On Diff #192273) | minor nit: no blank line between the 2 namespace openings. |
49 ↗ | (On Diff #192273) | blank again. |
llvm/lib/ObjectYAML/XCOFFYAML.cpp | ||
20 ↗ | (On Diff #192273) | nit: remove white-space. |
LGTM with minor changes.
llvm/lib/Object/XCOFFObjectFile.cpp | ||
---|---|---|
121 ↗ | (On Diff #192273) | Minor nit: Naming is not consistent (ret versus Result). |
llvm/lib/ObjectYAML/XCOFFYAML.cpp | ||
17 ↗ | (On Diff #192273) | I am not seeing a need for <cstdint> or <vector>. At the same time, <string.h> should be added for ::memset. |
April:
- added some blank line and deleted some blank line as suggest.
- deleted some header files #include "llvm/ObjectYAML/YAML.h"
#include "llvm/Support/YAMLTraits.h"
#include <cstdint>
from xcoffyaml.cpp
llvm/include/llvm/Object/XCOFFObjectFile.h | ||
---|---|---|
108 ↗ | (On Diff #192273) | done |
llvm/include/llvm/ObjectYAML/XCOFFYAML.h | ||
19 ↗ | (On Diff #192273) | deleted , thanks |
49 ↗ | (On Diff #192273) | added a blank line , thanks |
llvm/lib/ObjectYAML/XCOFFYAML.cpp | ||
17 ↗ | (On Diff #192273) | I deleted the |
20 ↗ | (On Diff #192273) | done |
llvm/include/llvm/ObjectYAML/XCOFFYAML.h | ||
---|---|---|
12 ↗ | (On Diff #192273) | done,thanks |
llvm/lib/Object/XCOFFObjectFile.cpp | ||
121 ↗ | (On Diff #192273) | change to Result |
llvm/lib/ObjectYAML/XCOFFYAML.cpp | ||
17 ↗ | (On Diff #192273) | I think <string.h> is already be included in the headers files which be included in #include "llvm/ObjectYAML/XCOFFYAML.h" ,and I tried to use -E of g++ to preprocessor the file , I can find the string.h in the preprocessor file. |
llvm/lib/ObjectYAML/XCOFFYAML.cpp | ||
---|---|---|
17 ↗ | (On Diff #192273) | Firstly, is ::memset part of the abstract interface (as opposed to an implementation-related detail) of any of the headers included directly by this file? I believe the answer is "no". Secondly, is <string.h> actually included by any of the headers in the LLVM project that are transitively included from this file? Of the 51 headers, none of them include <string.h> (although some include <cstring>, which does not have to declare ::memset). The 51 headers I checked are: |
@DiggerLin; as discussed off-list, I'll commit this for you with the additional change to include <string.h>.
return; can be deleted