[PATCH] D33566: Adding parsing ability for .res file, building the resource directory tree

[Zachary Turner via Phabricator via llvm-commits]

zturner accepted this revision.
zturner added a comment.
This revision is now accepted and ready to land.

lgtm with the last two suggested changes.



================
Comment at: llvm/lib/Object/WindowsResource.cpp:80
   RETURN_IF_ERROR(Reader.readInteger(HeaderSize));
+
+  if (HeaderSize < MIN_HEADER_SIZE)
----------------
ecbeckmann wrote:
> zturner wrote:
> > ecbeckmann wrote:
> > > zturner wrote:
> > > > I think this function would be a lot clearer if we had some structs that we could just read out in one or two lines rather than reading some fields, ignoring others, skipping sometimes, reading sometimes, etc.  How about this?
> > > > 
> > > > ```
> > > > class ResourceEntryRef {
> > > >   struct HeaderPrefix {
> > > >     ulittle32_t DataSize;
> > > >     ulittle32_t HeaderSize;
> > > >     ulittle16_t Unknown;
> > > >     ulittle16_t TypeID;
> > > >     ulittle16_t IDFlag;
> > > >     ulittle16_t NameID;
> > > >   };
> > > > 
> > > >   struct HeaderSuffix {
> > > >     ulittle32_t DataVersion;
> > > >     ulittle16_t MemoryFlags;
> > > >     ulittle16_t LanguageID;
> > > >     ulittle32_t Version;
> > > >     ulittle32_t Characteristics;
> > > >   };
> > > > 
> > > >   const HeaderPrefix *Prefix = nullptr;
> > > >   const HeaderSuffix *Suffix = nullptr;
> > > > };
> > > > 
> > > > ...
> > > > 
> > > > Error ResourceEntryRef::loadNext() {
> > > >   RETURN_IF_ERROR(Reader.readObject(Prefix));
> > > >   if (Prefix->IDFlag != 0xFFFF) {
> > > >     Reader.setOffset(Reader.getOffset() - 4);
> > > >     RETURN_IF_ERROR(Reader.readWideString(Name));
> > > >   }
> > > > 
> > > >   RETURN_IF_ERROR(Reader.readObject(Suffix));
> > > >   RETURN_IF_ERROR(Reader.readArray(Data, Prefix->DataSize);
> > > >   RETURN_IF_ERROR(Reader.padToAlignment(sizeof(uint32_t));
> > > >   return Error::success();
> > > > }
> > > > ```
> > > Hmmm this issue is actually pretty difficult because we can't really use a HeaderPrefix structure at all.  I didn't realize this when I first implemented this but actually both the Type and the Name can either be numeric IDs or variable length strings; therefore there isn't really any "standard" length that we can read into a structure easily.  Perhaps we can have a HeaderSuffix structure though....
> > I see.  In that case, can you add a function
> > 
> > ```
> > Error ResourceEntryRef::readStringOrId(BinaryStreamReader &Reader, uint16_t &ID, ArrayRef<UTF16> &Str);
> > ```
> > 
> > and then write:
> > 
> > ```
> > RETURN_IF_ERROR(readStringOrId(Reader, TypeID, Type));
> > RETURN_IF_ERROR(readStringOrId(Reader, NameID, Name));
> > ```
> > 
> Okay.  I feel like this function shouldn't be a member function of ResourceEntryRef because it only really has a use in this specific instance and it's precondition is that the offset of the reader has been directed to a specific point.
Seems reasonable.  But if it's going to be global, make it `static` so that its linkage doesn't leak outside of this TU.


================
Comment at: llvm/lib/Object/WindowsResource.cpp:102-104
   // The data and header size ints are themselves part of the header, so we must
   // subtract them from the size.
+  HeaderSize -= 2 * sizeof(uint32_t);
----------------
It looks like this is no longer being used, so we can probably delete this line.


https://reviews.llvm.org/D33566