This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] Add helper functions `printVersionSymbol()`, `printVersionDefinition()` and `printVersionDependency()`
AbandonedPublic

Authored by Higuoxing on Apr 5 2019, 8:43 AM.

Details

Summary

Since LLVM Style dumpers and GNU style dumpers share some same logic, we could simplify some logic inside printing functions.

Event Timeline

Higuoxing created this revision.Apr 5 2019, 8:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2019, 8:43 AM
Higuoxing updated this revision to Diff 193893.Apr 5 2019, 8:47 AM

Fix one small nit.

Higuoxing updated this revision to Diff 193895.Apr 5 2019, 8:58 AM

Move VersymBuf out of printVersionSymbol().

Higuoxing added inline comments.Apr 6 2019, 8:38 PM
llvm/tools/llvm-readobj/ELFDumper.cpp
4578–4579

Here, I changed a behavior of this tool. In this patch, if no versioning section appeared, we won't print anything. Before this patch, we will print:

Version symbols {
}
SHT_GNU_verdef {
}
SHT_GNU_verneed {
}

I don't know if this could be accepted

Higuoxing planned changes to this revision.Apr 8 2019, 5:49 AM

This patch breaks some tests, I will modify this later

Higuoxing updated this revision to Diff 195355.EditedApr 16 2019, 4:42 AM

Sorry for not responding on this patch for a long time.

I have tried several methods to simplify some shared logic of LLVMStyle and GNUStyle.

In this patch, I add another 3 functions to place some shared logic or variables. I met a problem which is, in LLVMStyle, we should print empty closure for empty versym, verneed, verdef sections. But DictScope cannot be used to forward-declare. For example:

DictScope D; // to extend D's lifetime.
if (opts::Output == opts::LLVM)
  D = DictScope(W, "XXX");
if (!Sec)
  return;

So, I have to write empty closure by hand. Like:

if (!Sec) {
  if (opts::Output == opts::LLVM)
    W.startLine() << "ScopeName {\n}\n";
  return;
}

In lld, some tests detect the empty closures, so we have to keep this feature.

As I'll be away next week, I've added a couple of others to help with reviewing this too.

I'm not certain that the approach you've taken is quite right. There should be no need to query the opts::Output member from within the dumper. That's the purpose of the individual dump styles. There's barely any common code you've actually pulled out. What is the purpose of what you have done?

llvm/test/tools/llvm-readobj/elf-empty-version-info.test
2

What about a test for the llvm-readelf behaviour?

19–21

Can you get rid of the Sections: block?

llvm/tools/llvm-readobj/ELFDumper.cpp
354

This should be plural (i.e. printVersionSymbols) since it will print more than one symbol. The same goes for the other functions below (Definitions and Dependencies).

Higuoxing planned changes to this revision.Apr 16 2019, 9:04 AM

As I'll be away next week, I've added a couple of others to help with reviewing this too.

I'm not certain that the approach you've taken is quite right. There should be no need to query the opts::Output member from within the dumper. That's the purpose of the individual dump styles. There's barely any common code you've actually pulled out. What is the purpose of what you have done?

Sure, I will try to get rid of it.

As I'll be away next week, I've added a couple of others to help with reviewing this too.

Thanks a lot. I will discuss with them.

Higuoxing abandoned this revision.May 28 2019, 11:02 PM