This is an archive of the discontinued LLVM Phabricator instance.

Add --print-icf-sections flag
ClosedPublic

Authored by gbreynoo on Jan 22 2018, 7:31 AM.

Details

Summary

Currently ICF information is output through stderr if the "--verbose" flag is used. This differs to Gold for example, which uses an explicit flag to output this to stdout. This diff adds the "--print-icf-sections" and "--no-print-icf-sections" flags and changes the output message format for clarity and consistency with "--print-gc-sections". These messages are still output to stderr if using the verbose flag. However to avoid intermingled message output to console, this will not occur when the "--print-icf-sections" flag is used.

Existing tests have been modified to expect the new message format from stderr.

Diff Detail

Repository
rL LLVM

Event Timeline

gbreynoo created this revision.Jan 22 2018, 7:31 AM
gbreynoo updated this revision to Diff 130904.Jan 22 2018, 8:33 AM

Updated tests that used existing icf output

ruiu added inline comments.Jan 22 2018, 11:57 AM
ELF/Driver.cpp
647 ↗(On Diff #130904)

hasFlag ( -> hasFlag(

ELF/ICF.cpp
436–440 ↗(On Diff #130904)

Can you define this function

static void print(const Twine &S) {
  if (Config->PrintIcf)
    message(S);
  else
    log(S);
}

and use it?

gbreynoo updated this revision to Diff 131075.Jan 23 2018, 7:55 AM

Changes following Rui's comments

gbreynoo updated this revision to Diff 131092.Jan 23 2018, 9:09 AM

Changes following Rafael's comment on the mailing list:
"gold calls this option --print-icf-sections, why not use the same name?"

grimar added a subscriber: grimar.Jan 24 2018, 12:48 AM
grimar added inline comments.
test/ELF/print-icf.s
7 ↗(On Diff #131092)

I think it is better to use explicit way of checking the output:

  1. RUN: ld.lld %t %t1 -o %t2 --icf=all --print-icf-sections --no-print-icf-sections | FileCheck %s --check-prefix=PREFIX
  2. PREFIX-NOT: selected
  3. PREFIX-NOT: removing

Otherwise test can pass by mistake.

gbreynoo updated this revision to Diff 131772.Jan 29 2018, 5:17 AM

Changes following George and Rafaels comments.

gbreynoo updated this revision to Diff 131987.EditedJan 30 2018, 9:37 AM

Changes following Rafael's comment:

+ auto PrintICF = [&](const Twine &prefix, size_t I) {

prefix -> Prefix

+ const auto *F = Sections[I]->File;
+ Twine S = prefix + " section '" + Sections[I]->Name + "' from file '" +
+ (F ? Twine(F->getName()) : "<internal>") + "'";

It is in general dangerous to use a Twine Value. I would suggest
writting this as:

if (!Config->Verbose && !Config->PrintIcfSections)
return;

std::string S = ....;

ruiu added inline comments.Jan 30 2018, 10:35 AM
ELF/ICF.cpp
427 ↗(On Diff #131987)

Everything in this file is related to ICF, so "ICF" is redundant unless it is in the global namespace. I'd name this just Print.

430 ↗(On Diff #131987)

Please avoid auto unless the actual type is obvious.

Maybe this code is a bit better.

std::string Filename = Sections[I]->File ? Sections[I]->File->getName() : "<internal>";
431–433 ↗(On Diff #131987)

I believe you can do this (which creates a Twine and call its str() function).

std::string S = (Prefix + " section '" + Sections[I]->Name +
                  "' from file '" + (F ? F->getName() : "<internal>") +
                  "'").str();
gbreynoo updated this revision to Diff 132133.Jan 31 2018, 4:09 AM

Changes following Rui's comments

gbreynoo updated this revision to Diff 132140.Jan 31 2018, 4:29 AM

Change following Rui's comments, missed in last diff

ruiu accepted this revision.Jan 31 2018, 3:01 PM

LGTM

This revision is now accepted and ready to land.Jan 31 2018, 3:01 PM
gbreynoo edited the summary of this revision. (Show Details)Feb 1 2018, 6:17 AM
gbreynoo retitled this revision from Add --print-icf flag to Add --print-icf-sections flag.Feb 1 2018, 6:41 AM
gbreynoo edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.