This is an archive of the discontinued LLVM Phabricator instance.

Boilerplate for modifications to the readobj tool to display .rsrc
ClosedPublic

Authored by ecbeckmann on Apr 24 2017, 6:19 PM.

Details

Summary

Adding functionality to readobj to display resource section data. This is necessary in order to test the cvtres tool when it is added.

Event Timeline

ecbeckmann created this revision.Apr 24 2017, 6:19 PM
zturner edited edge metadata.Apr 24 2017, 6:38 PM

I think this is a bit too small of a change. I know I said to do it incrementally and not as one big patch, but it seems like we should be able to do something meaningful. For example, there's a .rsrc$1 and .rsrc$2, perhaps you could display some metadata about them and dump the raw bytes of the sections without parsing the tree structure and individual resources.

Adding some basic functionality for displaying timestamps of .rsrc directory table.

ecbeckmann updated this revision to Diff 96646.Apr 25 2017, 4:22 PM

Prints timestamp and actual data of .rsrc sections.

ecbeckmann updated this revision to Diff 96650.Apr 25 2017, 4:50 PM

Fixed some descriptions, removed logging

Since this is printing something real now, can we add a FileCheck test for it?

llvm/include/llvm/Object/COFF.h
1064–1066

Seems like either all fields should be initialized inline, or none should. Is there a reason only OwningObject gets the special treatment?

llvm/lib/Object/COFFObjectFile.cpp
454

delete this debug statement as well as the subsequent ones as well.

llvm/tools/llvm-readobj/COFFDumper.cpp
1536–1537

This will miss a couple of cases.

  1. .rsrc. This occurs in the merged executable file. For example:
D:\src\llvmbuild\ninja>bin\llvm-objdump -section-headers bin\clang.exe

bin\clang.exe:  file format COFF-i386

Sections:
Idx Name          Size      Address          Type
            ...
  6 .rsrc         0000075c 0000000006bb2000 DATA
            ...
  1. It's possible in the future we may end up finding out there's a .rsrc$03. I'd change this check to if (!Name.starts_with(".rsrc"))
1540–1541

Move these two lines up out of the if statement.

1544–1549

It looks like this is a time_t? We have some functions so you don't have to reproduce this formatting code every time.

#include "llvm/Support/Chrono.h"

...

auto Time = sys::toTimePoint(TimeDateStamp);
SmallString<20> FormattedTime;
raw_svector_ostream OS(FormattedTime);
OS << Time;

W.printHex("Time/Date Stamp", FormattedTime, TimeDateStamp);
1553–1555

Then delete this branch and call printBinaryBlock below.

zturner requested changes to this revision.Apr 25 2017, 4:55 PM
This revision now requires changes to proceed.Apr 25 2017, 4:55 PM
ruiu added inline comments.Apr 25 2017, 9:39 PM
llvm/tools/llvm-readobj/COFFDumper.cpp
1542–1543

I don't see why you need this ResourceDirTableRef class. Since you are accessing section data by casting it to coff_resource_dir_table *, you can just access members directly. I mean, you could do something like this.

auto *Ent = reinterpret_cast<const coff_resource_dir_table *>(Ref.data()));
...
time_t TDS = time_t(Ent->TimeDateStamp);

This is I think the usual way of accessing COFF section data.

ecbeckmann edited edge metadata.
ecbeckmann marked 4 inline comments as done.

Update to print .rsrc in both .o and .exe

ecbeckmann added inline comments.Apr 26 2017, 11:20 AM
llvm/include/llvm/Object/COFF.h
1064–1066

This pattern is followed for all other section wrapper classes in this file. I don't understand the inherent benefit of doing this but I see no reason to break the pattern either.

llvm/tools/llvm-readobj/COFFDumper.cpp
1536–1537

Ah okay...I was thinking for now we can just build the feature around reading .o's and not .exe's because that's what we want to test, but if it's not much more effort we might as well do both.

1542–1543

True, however in the coming patches I think the ResourceDirTableRef class will be extended to do many more complicated things, like traversing the tree structure and returning certain resource data. I think it will be good to encapsulate this.

1544–1549

Done, however it looks like using the TimePoint library gives times based on 1969-12-31 instead of 1970-1-1, is this expected behavior?

ruiu added inline comments.Apr 26 2017, 11:25 AM
llvm/include/llvm/Object/COFF.h
1064–1066

Adding a wrapper that you don't need and even understand existential reason is not a good idea. You can add it later if you need it, but at the moment this new code just complicates the code unnecessarily.

zturner added inline comments.Apr 26 2017, 11:28 AM
llvm/tools/llvm-readobj/COFFDumper.cpp
1544–1549

Hmm, I find this a bit surprising. Looking at the code, it calls std::chrono::system_clock::from_time_t which takes a std::time_t. cppreference says that it's not technically defined, but almost always uses the unix epoch. I would expect that your implementation is also using the unix epoch. So maybe it's a problem in the printing code?

Can you paste the exact formatted string both with the original code and the operator<< code?

ruiu edited edge metadata.Apr 26 2017, 11:32 AM

I seems like a timezone issue. We are in PST, and PST is UTC-0800, which describes why you got 1969-12-31 16:00:00 instead of 1970-00-00 00:00:00. You want to print it out in UTC.

ecbeckmann updated this revision to Diff 96810.Apr 26 2017, 1:15 PM
ecbeckmann marked an inline comment as done.

Removed the unnecessary wrapper class for now, updated printing time stamp.

In D32463#738457, @ruiu wrote:

I seems like a timezone issue. We are in PST, and PST is UTC-0800, which describes why you got 1969-12-31 16:00:00 instead of 1970-00-00 00:00:00. You want to print it out in UTC.

The new code should print the correct time zone.

zturner accepted this revision.Apr 26 2017, 1:33 PM

looks good, thanks for being patient :)

This revision is now accepted and ready to land.Apr 26 2017, 1:33 PM
ruiu added a comment.Apr 26 2017, 1:35 PM

This looks much better now. A few minor nits.

llvm/tools/llvm-readobj/COFFDumper.cpp
1545

I think strftime null-terminates a string, so you don't need to initialize the buffer with = {}.

1547

sizeof(FormattedTime) is better than 20.

ecbeckmann updated this revision to Diff 96813.Apr 26 2017, 1:44 PM
ecbeckmann marked 2 inline comments as done.

Fixed some nits.

ruiu accepted this revision.Apr 26 2017, 1:46 PM

LGTM

ecbeckmann abandoned this revision.Apr 27 2017, 11:36 AM
This revision was automatically updated to reflect the committed changes.