• Fix error message punctuation

In D58316#1401185, @jhenderson wrote:

I guess this is okay, but I'm wondering if it's gone too far? The majority of cases you've changed here are in the driver layer, so wouldn't likely ever be librarified. What's the motivation for this round?

In D58296#1400258, @MaskRay wrote:

The approach looks good to me, but is anyone concerned about the 3MB compressed file?

FWIW, this would be the 11th largest file:

• vector->ArrayRef
• Pluralize Sections arg

Just a couple comments, but feel free to commit once those are fixed. Thanks!

I think you can still add tests for the other options, e.g. test/tools/llvm-objdump/X86/macho-disassembly-g-dsym.test tests -d -g, so you could add another run statement to that test with -dg and use cmp to verify it produces the same thing

Thanks!

In D56508#1397156, @void wrote:

This fails if the archives are nested. E.g.:

$ mkdir -p foo/bar
$ cat > foo/bar/a.c <<EOF
int bar() { return 42; }
EOF
$ rm -f built-in.o foo/built-in.o foo/bar/built-in.o ; llvm-ar rcSTD foo/bar/built-in.o foo/bar/a.o ; llvm-ar rcSTD foo/built-in.o foo/bar/built-in.o ; llvm-ar rcSTD built-in.o foo/built-in.o
./llvm.opt.install/bin/llvm-ar: error: 'a.o': No such file or directory.

What's happening is that it's trying to find a.o in foo/ when it's really in foo/bar/.

I cross-referenced the values from https://github.com/hjl-tools/x86-psABI/wiki/x86-64-psABI-draft.pdf

• Use optional
• Fix error message casing

In D57896#1391925, @hubert.reinterpretcast wrote:

In D57896#1391611, @zturner wrote:

Is this actually any better? Whereas before we can’t differentiate type names and variable names, under this proposal we can’t differentiate type names and function names. So it seems a bit of “6 of 1, half dozen of another”

Perhaps you mistyped? The proposal does not change the status quo of either type names nor function names. If you mean that we can't differentiate variable names and function names, then it seems worthwhile to point out that the actual letters (not just the case of said letters) matter too. Whereas the guidelines state that types and variables should have names that are nouns, the guidelines state that functions should have names that are verb phrases.

• Add --fallback-debug-path flag for testing /usr/lib/debug fallback

Reopened + updated patch with LibDriver fixes. I believe this should fix the chromium build, but haven't verified it. The new test case (llvm/test/tools/llvm-lib/thin-relative.test) should capture the issue, and was failing with the old patch but is now fixed.

Add regression test for chromium linking issue, and update LibDriver call site to correctly compute relative paths.

In D57842#1390249, @hans wrote:

I reverted this in r353507 as it broke the Chromium build; see https://crbug.com/930058

In D57927#1389723, @ruiu wrote:

I played with ar and found that the following behavior is pretty odd.

1. Create an archive $ mkdir x $ echo 123 > x/foo.txt $ ar rcS archive.a x/foo.txt
2. Directory name is not stored to archive.a $ cat archive.a !<arch> foo.txt/ 0 0 0 644 4 ` 123
3. But you can extract foo.txt with any directory name $ ar x archive.a bar/foo.txt $ cat foo.txt 123
   
   If I understand correctly, ar by default ignores directory names in many situations like the above, and "P" flag is to make it to not ignore directory names in a pathname. It looks like the default behavior is broken. Is that important for compatibility? I wonder if there's a chance to make "P" the default behavior.

In D57609#1389282, @rupprecht wrote:

In D57609#1387458, @samsonov wrote:

Ugh, too bad the original commit didn't provide any rationale, but here's my guess based on reading the source code:

Note that llvm-symbolizer (or another tool that uses this library) can be invoked with the relative path (e.g. cd /tmp/dir; llvm-symbolizer foo).

Then, if you can't find the .debuglink file at following locations: foo.debuglink (absolute path /tmp/dir/foo.debuglink), or .debug/foo.debuglink (absolute path /tmp/dir/.debuglink/foo.debuglink),
which can often happen if you're symbolizing the code from standard library, you have to look for this file in /usr/lib/... or /usr/libdata/... for NetBSD. That's when you need the realpath for the file in question,
so that you try to open:

/usr/lib/debug/tmp/dir/foo.debuglink

instead of

/usr/lib/debug/foo.debuglink

I think the absolute path (not realpath) would suffice, then.

Also, using the full path can be delayed to when checking in /usr/lib/debug.

I'll restore this patch with those modifications. No idea how I'll create a test for it though -- writing to /usr/lib/debug probably doesn't work on buildbots.

In D57609#1387458, @samsonov wrote:

Ugh, too bad the original commit didn't provide any rationale, but here's my guess based on reading the source code:

Note that llvm-symbolizer (or another tool that uses this library) can be invoked with the relative path (e.g. cd /tmp/dir; llvm-symbolizer foo).

Then, if you can't find the .debuglink file at following locations: foo.debuglink (absolute path /tmp/dir/foo.debuglink), or .debug/foo.debuglink (absolute path /tmp/dir/.debuglink/foo.debuglink),
which can often happen if you're symbolizing the code from standard library, you have to look for this file in /usr/lib/... or /usr/libdata/... for NetBSD. That's when you need the realpath for the file in question,
so that you try to open:

/usr/lib/debug/tmp/dir/foo.debuglink

instead of

/usr/lib/debug/foo.debuglink

In D56943#1388985, @rupprecht wrote:

In D56943#1388314, @kristina wrote:

The patch itself looks sound. However given that you have a specific use case in mind (TableGen files) could you provide supplementary coverage for that specific use case (unit tests for formatting td syntax using format::getLLVMStyle(format::FormatStyle::LK_TableGen)? I'm not entirely sure how useful this particular change is given that there's no linked patches related to your use case, I think adding those would help as well (possibly as a separate dependent patchset).

The use case is a pre-refactoring for D55964, which is in the patch description -- is that what you mean by "linked patches"?
I'll update that patch to use this approach, and make it a dependent patch.

• Rebased w/ D56943 patched in so we can override just TableGen in getLLVMStyle()

In D56943#1388314, @kristina wrote:

The patch itself looks sound. However given that you have a specific use case in mind (TableGen files) could you provide supplementary coverage for that specific use case (unit tests for formatting td syntax using format::getLLVMStyle(format::FormatStyle::LK_TableGen)? I'm not entirely sure how useful this particular change is given that there's no linked patches related to your use case, I think adding those would help as well (possibly as a separate dependent patchset).

ping

• Fix related test to always delete the archive first
• Inline addToStringTable
• Remove redundant assignment

• Move computeRelativePath special case from libObject to llvm-ar
• Remove obsolete test fixme

In D57700#1385476, @mattd wrote:

In D57700#1384786, @jhenderson wrote:

Hello Davide,
The present format is good enough to convey the information. It is however not sufficient or sometimes difficult for present users of GNU tools (which are quite ubiquitous) to make a
switch as it is complete redesign of the post processing tools they may have based on.

-Hemant

It's not immediately clear from the original introduction of GNU style whether complete output identity was the original goal, or just similarity. Similarly, it's not clear from my previously attempt to gather information as to users expectations. I've subscribed @khemant in case he's got any further comments, since he made the original change. My reading of rL260430 which commits it is that it is intended to be like GNU readelf, but doesn't necessarily have to be identical. I'm open to further discussion on this (and I actually have a BoF on this subject at the next Euro LLVM, but that's still a couple of months away).

@jhenderson Thanks for tracking down that comment. I interpreted that comment as you described. Personally, I think a one line addition to the existing GNU style output is a reasonable change that will not detract users who might be expecting GNU-styled output. The change provides the user with the familiar GNU style, and provides a one line bonus (it provides a complete table (all of the sections are accounted for)). The main "issue" I see is that anyone diffing llvm-readelf output against the equivalent GNU output will see a line difference (llvm-readelf has the Notes, GNU readelf does not). If users diff output between llvm-readelf and GNU readelf, they would see other differences as well, primarily the number of digits used to represent some of the values in the --program-headers output.