In D75742#1912140, @jhenderson wrote:

Simplify size handling slightly.

I've not deleted the test yet, but I will do so in due course before this gets committed. I'd like to see if we can come up with an alternative test option first, if we can. For the record, the test passes locally with my patch, but didn't without the fix (it was how I found the bug in the first place).

• Annotate SecName param
• Use <?> for unknown section names

Upload correct patch

In D75658#1906729, @MaskRay wrote:

The change should be merged into D75647 because currently Name is not dumped.

• Update test comments

Is there a particular use case for this feature?

• clang-format
• Simplify test case

• Add demangling to match updated GNU objdump behavior

Thanks!

• Fix printing the same line info when only the function name changes (from something to <invalid>)

• Add back blank line accidentally removed
• Add source of IR

Sure, here's an example you can play around with:

• Revert back to UseSymbolTable = true to allow printing the name in binaries without debug info

• Use early return
• Add name mangling test coverage

In D74477#1873998, @andrewng wrote:

In my modified version of widenPath...

I second grimar's request for an associated test that covers this behavior, but are you saying that the issue is your modified version of sys::path::widenPath breaks some llvm-ar tests, and switching to sys::windows::UTF8ToUTF16 will keep them passing? (If so, submitting w/o an additional test sounds fine to me)

Thanks James!

In D72992#1871939, @thakis wrote:

This broke tests everywhere when the experimental AVR backend is enabled, e.g. http://45.33.8.238/

Please take a look, and if it takes a while to fix please revert while you investigate.

Oh, I missed that James already posted to llvm-dev: http://lists.llvm.org/pipermail/llvm-dev/2020-February/139115.html. Please reply there instead.

In D74433#1870647, @MaskRay wrote:

Wait. I wonder whether we can change llvm-readobj to use lower case names as well. The following should be updated:

StringRef ELFObjectFile<ELFT>::getFileFormatName() const {
  bool IsLittleEndian = ELFT::TargetEndianness == support::little;
  switch (EF.getHeader()->e_ident[ELF::EI_CLASS]) {
  case ELF::ELFCLASS32:
    switch (EF.getHeader()->e_machine) {
    case ELF::EM_386:
      return "ELF32-i386";
    case ELF::EM_IAMCU:
      return "ELF32-iamcu";
    case ELF::EM_X86_64:
      return "ELF32-x86-64";
    case ELF::EM_ARM:

These are almost bfdnames, except that they use upper cases. I don't find a compelling argument using upper case ELF, so I vote for changing these ELF to elf. @jhenderson @grimar @sbc100 thoughts?

// lib/Object/WasmObjectFile.cpp
StringRef WasmObjectFile::getFileFormatName() const { return "WASM"; }

• Use StringRef::lower()

In D71151#1813772, @labath wrote:

Judging by the direction of the lua patches, it doesn't look like we will have lua "dotest" tests, so I think it's fine to just put this into API directly (without the extra "test" subfolder").

It's also dependent on the build mode: https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/ErrorHandling.h#L134

FYI - reverted this too since I had conflicts when also reverting D74070.

FYI, temporarily reverted this in fafddbd956dbe439787f6d717c247e648bb07ff5 since it was causing failures in Clang :: Driver/crash-report.c. I didn't see the failure on all buildbots (otherwise I could have just marked it XFAIL), though I may have been looking in the wrong place.

LGTM after James' comments too. Thanks for spotting this inconsistency.

In D73318#1839196, @saugustine wrote:

In D73318#1838181, @jhenderson wrote:

Is there a particular reason to restrict this to GNU style? I think it's okay to expand on the LLVM style too, but might be worth getting more opinions on it.

LLVM output style already prints the column number (file:line:column), which tells you the same thing as the discriminator, but at a higher level that is more friendly for users. I'm open to adding it, but it seems a bit redundant.

Otherwise updated.

In D52810#1818001, @MaskRay wrote:

In D52810#1817883, @sbc100 wrote:

I it seems that the printing of this error message only occurs with GNU nm if the object doesn't have a symbols table at all. Have no symbols in the table does not print the error message.

$ touch foo.c
$ gcc -c foo.c
$ nm foo.o  <- no error output here.
$ strip foo.o
$ nm foo.o
nm: foo.o: no symbols

Interesting (subtle) behavior...

@rupprecht Do we know what projects parse "no symbols" from llvm-nm output?

I left similar comments on D72357 too

I left comments on D72357 instead

In D71554#1789360, @arichardson wrote:

Also handle -h/-v as short options. Does the adjusted test look okay?

Thanks -- the code in question seems to be heavily templated, so it may be a corner case with how those are handled. I'll revert shortly and follow up with some kind of repro, but may be delayed due to holidays.

We're seeing a large memory increase in compilations as a result of this patch: 4.7G -> 6.0G (25%)

In D71635#1790611, @MaskRay wrote:

Before:

% clang -frounding-math -fsyntax-only -x c /dev/null
clang-10: warning: Support for floating point control option frounding-math is incomplete and experimental [-Wexperimental-float-control]

CC1 will do rounding math things.

After

% clang -frounding-math -fsyntax-only -x c /dev/null
clang-10: warning: optimization flag '-frounding-math' is not supported [-Wignored-optimization-argument]

CC1 will not do rounding math things. -fexperimental-rounding-math if the user really wants to use the feature.

Is my understanding correct? If yes, this patch seems pretty reasonable to me, because -frounding-math is currently incomplete/unsafe.

You may consider not changing CC1 options as they are not user facing.

In D62731#1789122, @rupprecht wrote:

In D62731#1788902, @andrew.w.kaylor wrote:

In D62731#1788838, @rupprecht wrote:

It seems the discussion of whether or not this is incomplete died out -- I'd prefer to assume it is incomplete if there is no consensus. Mailed D71635 to rename -frounding-math to -fexperimental-rounding-math.

Alternatively we could remove the warning. I still don't see a good argument for the middle ground of having it called -frounding-math but also generate a warning.

It's definitely incomplete but the results will not be any worse than you get when -frounding-math is ignored.

My preference would be to change the text of the warning that is issued but allow -frounding-math to be enabled by this commit without requiring an additional option.

If other reviewers agree, then let's just remove the warning. I can send a patch tomorrow unless someone else wants to do that.

In D62731#1788902, @andrew.w.kaylor wrote:

In D62731#1788838, @rupprecht wrote:

It seems the discussion of whether or not this is incomplete died out -- I'd prefer to assume it is incomplete if there is no consensus. Mailed D71635 to rename -frounding-math to -fexperimental-rounding-math.

Alternatively we could remove the warning. I still don't see a good argument for the middle ground of having it called -frounding-math but also generate a warning.

It's definitely incomplete but the results will not be any worse than you get when -frounding-math is ignored.

My preference would be to change the text of the warning that is issued but allow -frounding-math to be enabled by this commit without requiring an additional option.

If other reviewers agree, then let's just remove the warning. I can send a patch tomorrow unless someone else wants to do that.

It seems the discussion of whether or not this is incomplete died out -- I'd prefer to assume it is incomplete if there is no consensus. Mailed D71635 to rename -frounding-math to -fexperimental-rounding-math.

Looks good for D/U, but looks like --help and --version options are also supported as combined short args; do you mind adding those too while you're here?

In D71035#1783196, @jhenderson wrote:

In D71035#1783026, @MaskRay wrote:

Is it ok for me to commit if there is still no other people interested in this patch for, say, 10 hours? 😂

I tested it on an internal objcopy -I ihex -O binary use case that motivated me to send the patch. With this patch, the section content layout is now the same.

Let's give it until after the weekend. We've been stung on a few occasions by issues with binary output, so I want to give people ample opportunity to point out anything else we've missed!

Looks good. Thanks for adding the tests!

In D71302#1778352, @MaskRay wrote:

In D71302#1778295, @rupprecht wrote:

Update the heuristic to make all the following work as intended:

llvm-ar-9
llvm-ranlib.exe
arm-pokymllib32-linux-gnueabi-llvm-ar
arm-pokymllib32-linux-gnueabi-llvm-ar-9

This was manually verified, but can you add a test for it?
You could probably write a lit test like:

# RUN: mkdir -p bin
# RUN: cp llvm-ar bin/llvm-ar-9
# RUN: bin/llvm-ar-9 --help | FileCheck %s --check-prefix=AR
# RUN: cp llvm-ar bin/llvm-ranlib.exe
# RUN: bin/llvm-ranlib.exe --help | FileCheck %s --check-prefix=RANLIB
...

The path of llvm-ar is not known.

lit substitutes llvm-ar with the full path; see llvm/test/tools/llvm-ar/case-detection.test