Page MenuHomePhabricator

[SystemZ][z/OS] Add support for TXT records in the GOFF reader
Needs ReviewPublic

Authored by yusra.syeda on Jun 1 2021, 1:41 PM.

Diff Detail

Event Timeline

yusra.syeda created this revision.Jun 1 2021, 1:41 PM
yusra.syeda requested review of this revision.Jun 1 2021, 1:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2021, 1:41 PM

ping :)

Sorry, I've been away, and this is pretty low on my review priority list, so I haven't had a chance to look at this until now.

llvm/include/llvm/Object/GOFF.h
95
103

What's the motivation for returning by updating an input parameter, rather than returning via the return type?

llvm/include/llvm/Object/GOFFObjectFile.h
33

There may be a perfectly valid reason for this, but why make this a friend class rather than just making the relevant functions public?

36

Any particular reason you've chosen the size 256 here and in various other places? It may deserve a comment or some sort of named constant.

70–71

What's the motivation for adding these two functions at this point? It seems like a bad idea to expand the interface without a use-case (and if there is a use-case, what is it)?

72

By name, this function sounds awfully like isSectionBSS (which for ELF at least means a section that is allocated space and zero-initialised at load time).

126

*Ref types are designed to be copied. Don't pass them as const &.

llvm/lib/Object/GOFFObjectFile.cpp
432

This TODO sounds a bit odd. What does the clang compiler have to do with this code?

478

Any particular reason you've used '\0' rather than simply 0? After all, you're storing it in a uint8_t (a number) rather than a char/unsigned char.

519–520

Why not simply return arrayRefFromStringRef(StringRef(Data));? In fact, could you get away with returning simply Data.data() or similar?

527

I'm pretty sure the right hand side of this function will return an int because that's the type of the literal 1 in this context. I think you want to do UINT64_C(1) to force the type of the literal (and therefore the result of the left-shift operation) to 64-bit.

539–540

Don't add unnecessary local variable. Same below for the other functions.

564–573
576–580

Why are you adding this function?

llvm/unittests/Object/GOFFObjectFileTest.cpp
507

You can omit the explicit 0 from here. This will zero-initialise the whole array.

587

Get rid of double blank line.

595
600–603

You should be able to just do EXPECT_EQ(Contents, "\x12\x34\x56\x78\x9a\xbc\xde\xf0");

Use EXPECT_EQ because one section's content not being right shouldn't impact other sections, same as with the SymbolName check above.

Additionally, you need checks that symbols() and sections() return the right number of symbols and sections somewhere. Your test would pass as things stand, if symbols() or sections() returned an empty list (and therefore no looping occurred).

Address comments in the unit test

yusra.syeda marked 4 inline comments as done.Jun 9 2021, 11:41 AM

Removed unused functions, and addressed some more review comments

yusra.syeda marked 11 inline comments as done.Jun 10 2021, 7:22 AM

Update the getElementEsdId function

yusra.syeda marked 2 inline comments as done.Jun 11 2021, 7:22 AM
yusra.syeda added inline comments.
llvm/include/llvm/Object/GOFF.h
103

There is no specific reason, I have changed it to return the type.

llvm/include/llvm/Object/GOFFObjectFile.h
33

It's also done this way in ELFObjectFile.h, so this follows

yusra.syeda marked 2 inline comments as done.Jun 11 2021, 7:22 AM
yusra.syeda added inline comments.Jun 14 2021, 12:29 PM
llvm/include/llvm/Object/GOFFObjectFile.h
36

An initial vector size of 256 guarantees that in most cases the vector does not need to be extended, and avoids a memory allocation in SmallVector.

jhenderson added inline comments.Jun 15 2021, 1:24 AM
llvm/include/llvm/Object/GOFFObjectFile.h
36

The trade-off is that this requires a significantly bigger stack allocation than might be necessary. How many TextPtrs might a typical file have?

llvm/lib/Object/GOFFObjectFile.cpp
519–520

In fact, could you get away with returning simply Data.data() or similar?

Any response to this part of the comment? I'm pretty sure you could just do return {Err.data(), Err.size()} without needing to jump through the StringRef constructor.

543–544

As previously requested in my above comment ("same below...") don't add unnecessary local variables like this.

Really, I'd expect this return line to just be return ESDREcord::getExecutable(EsdRecord) == GOFF::ESD_EXE_DATA;. I don't know why you have several functions that return via input parameter rather than normal C++ way of using the return type...

llvm/unittests/Object/GOFFObjectFileTest.cpp
587

size_t seems more appropriate for counting the number of something.

593

Prefer pre to postincrement. However, I think I have a beter suggestion, if you only expect one symbol:

auto Symbols  = GOFFObj->symbols();
ASSERT_EQ(std::distance(Symbols.begin(), Symbols.end(), 1);
SymbolRef Symbol = *Symbols.begin();
Expected<StringRef> SymbolNameOrErr = GOFFObj->getSymbolName(Symbol);
ASSERT_THAT_EXPECTED(SymbolNameOrErr, Succeeded());
StringRef SymbolName = SymbolNameOrErr.get();
EXPECT_EQ(SymbolName, "var#c");

A similar change would work for the section check below.

Updated the unit test and cleaned up return statements

yusra.syeda marked 3 inline comments as done.Jun 15 2021, 2:36 PM
yusra.syeda added inline comments.
llvm/lib/Object/GOFFObjectFile.cpp
519–520

Seems I can do something like this to avoid the StringRef constructor:

ArrayRef<uint8_t> temp(reinterpret_cast<const unsigned char *>(Data.data()), Data.size());
return temp;

If you prefer this, I can update it.

jhenderson added inline comments.Jun 16 2021, 12:17 AM
llvm/lib/Object/GOFFObjectFile.cpp
519–520

Ah, I see what some of the problem is now - didn't realise this returned an ArrayRef<uint8_t> rather than an ArrayRef<char>. Seems to me like this might be because your interfaces are inconsistent - some are using strings (effectively arrays of char), and others are using arrays of uint8_t). Is it possible to change your methods to canonicalise on one? To me it would be better to be the uint8_t option, since that is less likely to fall foul of signed conversion issues, and it's the closest thing to a true byte type that we have.

I think leaving as-is is fine, if it's not practical to canonicalise. For reference, you shouldn't need the temporary in your example, and should be able to just return it directly.

yusra.syeda marked 4 inline comments as done.Jun 16 2021, 1:43 PM
yusra.syeda added inline comments.
llvm/include/llvm/Object/GOFFObjectFile.h
36

The amount of TXT records can vary quite a bit. In an analysis of about 3390 object files, the average was about 1200 with the minimum being 3 and maximum being 376780. So 256 is a reasonable value.

llvm/lib/Object/GOFFObjectFile.cpp
519–520

Thanks, I will leave this as is for this patch.

yusra.syeda marked 2 inline comments as done.Jun 16 2021, 1:43 PM

Okay, no more comments from me at this time. Someone with GOFF knowledge needs to review the functionality for correctness.

kpn added a comment.Jun 17 2021, 12:32 PM

Say, I can't find any file in the llvm repository named "GOFF*". They don't show up on Github, either, so I'm not sure what's going on. Where is the GOFF support in the LLVM tree currently? There's no "https://github.com/llvm/llvm-project/tree/main/llvm/lib/Object/GOFFObjectFile.cpp", for example.

llvm/include/llvm/BinaryFormat/GOFF.h
152

This appears unused currently. Don't include it until the patch where you use it.

llvm/lib/Object/GOFFObjectFile.cpp
479

Is this check really needed? If a zero fill byte is wanted then the GOFF fill byte field will be zero.

Removed unused enum and a redundant if statement

Say, I can't find any file in the llvm repository named "GOFF*". They don't show up on Github, either, so I'm not sure what's going on. Where is the GOFF support in the LLVM tree currently? There's no "https://github.com/llvm/llvm-project/tree/main/llvm/lib/Object/GOFFObjectFile.cpp", for example.

This patch is based off of https://reviews.llvm.org/D98437 which is where files you mentioned are added. https://reviews.llvm.org/D98437 cannot be committed until https://reviews.llvm.org/D88741 is committed. So these files are not in the llvm repository yet.

yusra.syeda marked an inline comment as done.Jun 28 2021, 1:37 PM
yusra.syeda added inline comments.
llvm/lib/Object/GOFFObjectFile.cpp
479

No, it's not needed. I've updated this.

yusra.syeda marked an inline comment as done.Jun 28 2021, 1:37 PM
kpn added a comment.Jul 12 2021, 11:25 AM

Say, I can't find any file in the llvm repository named "GOFF*". They don't show up on Github, either, so I'm not sure what's going on. Where is the GOFF support in the LLVM tree currently? There's no "https://github.com/llvm/llvm-project/tree/main/llvm/lib/Object/GOFFObjectFile.cpp", for example.

This patch is based off of https://reviews.llvm.org/D98437 which is where files you mentioned are added. https://reviews.llvm.org/D98437 cannot be committed until https://reviews.llvm.org/D88741 is committed. So these files are not in the llvm repository yet.

I don't think we should be getting so far ahead of ourselves. Get your prerequisites in tree first and then we can proceed with this patch. That's my take, anyway.