Page MenuHomePhabricator

[ELF] Add missing options to ld.lld.1
ClosedPublic

Authored by MaskRay on Jul 27 2018, 2:13 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Jul 27 2018, 2:13 PM
MaskRay updated this revision to Diff 157769.Jul 27 2018, 2:33 PM

Add more options

ruiu added a reviewer: pcc.Jul 27 2018, 2:55 PM
ruiu added inline comments.
ELF/Options.td
61 ↗(On Diff #157769)

Add "(default") to this line just like we do for other default options.

docs/ld.lld.1
43 ↗(On Diff #157769)

Is the vertical bar the right notation? Please mimic other options that have aliases.

MaskRay updated this revision to Diff 157782.Jul 27 2018, 3:02 PM
  • "Do not apply link-time values for dynamic relocations">;

+ "Do not apply link-time values for dynamic relocations (default)">;

MaskRay marked an inline comment as done.Jul 27 2018, 3:04 PM
MaskRay added inline comments.
docs/ld.lld.1
43 ↗(On Diff #157769)

Ideally, a newline will be the best (which is also used in ld.bfd.1)

But I don't know how to express that in groff_mdoc. A vertical bar compared with a comma is more clear here. --version, -v is the only example I find and I have updated it.

ruiu added a reviewer: emaste.Jul 27 2018, 3:09 PM

Ed might know how to format options with aliases.

pcc added inline comments.Jul 27 2018, 6:31 PM
docs/ld.lld.1
43 ↗(On Diff #157769)

The only way that I can see to do it is like this:

diff --git a/lld/docs/ld.lld.1 b/lld/docs/ld.lld.1
index c9662856b89..d44995c842d 100644
--- a/lld/docs/ld.lld.1
+++ b/lld/docs/ld.lld.1
@@ -26,19 +26,24 @@ It accepts most of the same command line arguments and linker scripts
 as GNU linkers.
 .Pp
 These options are available:
-.Bl -tag -width indent
+.Bl -tag -width indent -compact
+.Pp
 .It Fl -allow-multiple-definition
 Do not error if a symbol is defined multiple times.
 The first definition will be used.
+.Pp
 .It Fl -as-needed
 Only set
 .Dv DT_NEEDED
 for shared libraries if used.
+.Pp
 .It Fl -auxiliary Ns = Ns Ar value
 Set the
 .Dv DT_AUXILIARY
 field to the specified name.
+.Pp
 .It Fl -Bdynamic
+.It Fl -dy
 Link against shared libraries.
 .It Fl -Bstatic
 Do not link against shared libraries.
MaskRay updated this revision to Diff 157829.Jul 27 2018, 7:42 PM

Use -compact and add .Pp

MaskRay updated this revision to Diff 157830.Jul 27 2018, 8:02 PM

There should be no | now.

MaskRay marked 3 inline comments as done.Jul 27 2018, 9:10 PM
MaskRay updated this revision to Diff 157837.Jul 27 2018, 11:53 PM

Prefer --defsym f=1 to --defsym=f=1

For reference I had a discussion on markup for option aliases with Ingo Schwarze and he wrote:

The standard method for achieving similar output as in the GNU ld(1) manual page goes as follows (without the comments i'm inserting/appending, of course):

The options are as follows:
.Pp \" You need this if you use -compact on the next line.
.Bl -tag -width Ds -compact
.It Fl l Ar libName
.\" No paragraph break here.
.It Fl -library Ns = Ns Ar libName
Add archive file ...
.Pp \" You need this between items if you use -compact above.
.It Fl L Ar searchdir
.It Fl -library-path Ns = Ns Ar searchdir
Add path ...
.El
.Pp \" You always need this if some text follows, even without -compact.

Then again, if the the options all fit on one output line, i don't really see the need and would rather recommend simply:

The options are as follows:
.Bl -tag -width Ds
.It Fl l Ar libName , Fl -library Ns = Ns Ar libName
Add archive file ...
.It Fl L Ar searchdir , Fl -library-path Ns = Ns Ar searchdir
Add path ...
.El
.Pp

If you do this, always put the short option left of the long one, and decide alphabetic ordering by the short one, not by the long one.

emaste added inline comments.Jul 30 2018, 6:31 AM
docs/ld.lld.1
6 ↗(On Diff #157837)

Bump .Dd upon commit

63 ↗(On Diff #157837)

What is the reason for changing these from Ns = Ns? According to Ingo that's what the markup should be.

FWIW I don't have a strong opinion on | vs newline to separate aliases, but if we are going to go with the newline -compact and .Pp approach I'd suggest making that change as a separate commit first so that the actual content changes here are easier to see/review.

MaskRay updated this revision to Diff 157994.Jul 30 2018, 10:16 AM

Remove -compact and .Pp , put aliasing options on the same line

MaskRay marked an inline comment as done.Jul 30 2018, 10:17 AM

FWIW I don't have a strong opinion on | vs newline to separate aliases, but if we are going to go with the newline -compact and .Pp approach I'd suggest making that change as a separate commit first so that the actual content changes here are easier to see/review.

Done

If you do this, always put the short option left of the long one, and decide alphabetic ordering by the short one, not by the long one.

I agree with this rule generally for other software but... for linker options (with a lot of short and long options for various historical heritage, single/double dashes...) I still have the tendency not to prefer unusual short options, e.g. -h is a rather uncommon alias of -soname
Similarly, I want to put --strip-debug before -S. --discard-all before -x.

MaskRay updated this revision to Diff 158001.Jul 30 2018, 10:48 AM

Commit uncontroversial parts

MaskRay updated this revision to Diff 158008.Jul 30 2018, 10:58 AM

Minor style change to --omagic

ruiu added inline comments.Jul 30 2018, 12:58 PM
docs/ld.lld.1
39 ↗(On Diff #158008)

Why did you remove "="?

410 ↗(On Diff #158008)

Why did you swap the order?

MaskRay marked an inline comment as done.Jul 30 2018, 1:09 PM
MaskRay added inline comments.
docs/ld.lld.1
39 ↗(On Diff #158008)

https://github.com/freebsd/freebsd/blob/4b26291ed887f8245f4c355f224743bfde7120f7/usr.bin/clang/lld/ld.lld.1#L70 uses this form but there are also other places preferring option=bar.

If you think = is better than a space, I can change all of them (Eq options) to use =.

410 ↗(On Diff #158008)

The long option --version is used as the key here for alphabetical sorting. --version should be ordered before --version-script so their orders are swapped.

ruiu added inline comments.Jul 30 2018, 1:17 PM
docs/ld.lld.1
39 ↗(On Diff #158008)

The GNU's convention seems to always add = for long options that take arguments. I'd follow that convention, but can you leave them alone for now?

I think you changed too many things in a single patch. Can you do that step by step? I'd revert all but the change to add missing options.

MaskRay updated this revision to Diff 158064.Jul 30 2018, 2:13 PM

Remove changes replacing = with

MaskRay marked 3 inline comments as done.Jul 30 2018, 2:18 PM
ruiu added inline comments.Jul 30 2018, 2:20 PM
docs/ld.lld.1
390–392 ↗(On Diff #158066)

Please also revert all these line-moving changes.

MaskRay updated this revision to Diff 158072.Jul 30 2018, 2:28 PM

Remove line moving changes

ruiu added inline comments.Jul 30 2018, 2:33 PM
docs/ld.lld.1
234 ↗(On Diff #158072)

Don't remove these options from the manual.

MaskRay updated this revision to Diff 158079.Jul 30 2018, 2:40 PM

Add back --no-pie --enable-new-dtags --no-gc-sections

MaskRay marked 4 inline comments as done.Jul 30 2018, 2:41 PM
ruiu added inline comments.Jul 30 2018, 2:41 PM
docs/ld.lld.1
202 ↗(On Diff #158072)

This should be

Do not fold
.Ar symbol
during ICF.
MaskRay updated this revision to Diff 158084.Jul 30 2018, 2:45 PM

Use .Ar in --keep-unique

ruiu added inline comments.Jul 30 2018, 2:50 PM
docs/ld.lld.1
217 ↗(On Diff #158084)

Consistently add = between a long option and its argument.

MaskRay updated this revision to Diff 158087.Jul 30 2018, 2:53 PM

--library-path dir => --library-path=dir

MaskRay marked 2 inline comments as done.Jul 30 2018, 2:53 PM
MaskRay retitled this revision from [ELF] Update ld.lld.1 to [ELF] Add missing options to ld.lld.1.
MaskRay edited the summary of this revision. (Show Details)
ruiu added inline comments.Jul 30 2018, 2:57 PM
docs/ld.lld.1
204 ↗(On Diff #158087)

This is redundant. We don't say "this value" for other options that takes a "value", for example.

MaskRay updated this revision to Diff 158099.Jul 30 2018, 3:18 PM

Remove redundant "this"

ruiu added a comment.Jul 30 2018, 3:20 PM

LGTM

Please create a small patch like this one next time for ease of code review. Thanks!

ruiu accepted this revision.Jul 30 2018, 3:20 PM
This revision is now accepted and ready to land.Jul 30 2018, 3:20 PM
This revision was automatically updated to reflect the committed changes.