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

In D71151#1774586, @labath wrote:

While I'm not opposed to moving "lldbsuite" to some place closer to the tests, one thing I'd like to get out of this is to have the actual tests stand out more prominently.

Ditto; my direct goal is just to be able to run something more fine grained than ninja check-lldb-api, but splitting tests & test framework is a nice side benefit. (Actually it might be simpler if I didn't split it, but now I've already done the work).

In D71150#1774575, @labath wrote:

Hmm... given the lua thread on lldb-dev, maybe we should slow down here, and consider the potential relationship of these tests and the hypothetical lua tests that come to be. One of the reasons the current tests are so deeply nested was that Zach was trying to future-proof ourselves for addition of new scripting languages (at that time, Java was being floated around, but the idea never took off the ground). I'd hate to move these tests now, only to have to move them around again in a couple of weeks to make room for lua...

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:

In D71030#1769588, @MaskRay wrote:

Oh, I think this may break llvm-dlltool.exe and llvm-ar.exe on Windows. We probably need a more sophisticated heuristic.

Since the diff is large, I'll paste the only non-mechanical part here:

Officially abandoning this

In D70622#1771601, @rupprecht wrote:

In D70622#1765674, @JDevlieghere wrote:

No objections to moving the test. Jordan, let me know if you're up for it, otherwise I'm happy to take care of it.

I tried moving it today. The rabbit hole goes deep :D
The test framework seems pretty tied to the directory layout (because python), e.g. those must be in a directory called "lldbsuite" to be able to "import lldbsuite"
The test infrastructure also seems to assume tests + test framework are in the same tree (e.g. the LLDB_TEST env var is used for both), so moving the individual test directories and leaving just the framework classes in lldbsuite breaks a few things.

I agree it's probably cleaner to move the tests, so I'll keep poking. Suggestions welcome though.

In D70622#1765674, @JDevlieghere wrote:

No objections to moving the test. Jordan, let me know if you're up for it, otherwise I'm happy to take care of it.

In D70324#1755712, @davide wrote:

In D70324#1755704, @labath wrote:

[This is probably not the right patch, as the last run on that bot still doesn't have this commit.]

-> D70137 is the correct patch that actually adds this test -- this is testonly cleanup

• Fix test comment
• Use expect helper from lldbpexpect

In D70137#1742766, @JDevlieghere wrote:

This is very nice, thanks!

Any chance we could test this with a pexpect test?

• Add a pexpect test

I found a strange sanitizer error as a result of this revision, could you take a quick look at https://bugs.llvm.org/show_bug.cgi?id=43870?

It looks like this caused a very large increase in binary size (627M->686M). Is that expected/has anyone else observed this?

LGTM with the wildcard changes, thanks!

LGTM to the change, though I want to resolve this thread before committing:

Not really opposed to this patch, but I think ruiu's comment makes sense to think about:

In D69188#1716472, @thieta wrote:

@rupprecht thanks for the review - happy to address your comments and change the commit message and inline comment to reflect my findings above if you guys decide to accept this approach. I think this needs to be documented if nothing else considering how confusing the error on device actually is.

I'm good with the approach in this patch -- once the test/docs changes are done, I'll accept it. I also ran into Jake yesterday at the llvm devmtg and came to the same conclusion. (Also, sorry for the delay, which was also because of the devmtg). --strip-all should generally be more aggressive, but not so much that it totally doesn't work as a default mode on common platforms.

Also, a couple docs that should be updated (where we mention .gnu.warning):

In D69188#1715050, @thieta wrote:

In D69188#1715040, @abrachet wrote:

Seems like a hack, no? What about using --keep-section when you need it? You say its not strictly needed on modern systems and the problem is with the dlopen implementation of these older libcs, and not llvm-objcopy, so I would be hesitant to add something like this especially given we have a solution with --keep-section

Keep-sections totally works. But since this produces unusable binaries on systems that require this section and lld already makes this consession I think it's worth adding this very low maintenance hack.

I agree, we generally want --strip-all to be the aggressive option and --strip-all-gnu to be the option when more compatibility is needed (and --keep-section when things seriously go wrong), but I think we should probably not have --strip-all be completely unusable for default modes.

In D68064#1713535, @delcypher wrote:

Damn looks like something is broken for linux powerpc

http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/21208/steps/ninja%20check%201/logs/stdio

llvm-lit: /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/projects/compiler-rt/test/builtins/Unit/lit.cfg.py:116: fatal: builtins_source_features contains duplicates: ['librt_has_divtc3']

I'm going to turn this into a warning to unbreak the bot for now.

@joerg @rupprecht Any idea what's going on here? My CMake code jsut reads the source files used to built the builtin library. My guess is that both lib/builtins/ppc/divtc3.c and lib/builtins/divtc3.c are used to build the builtin library for powerpc. That sounds like a bug. I'd expect that to cause a linker error with multiple symbols of the same name being defined.

Generally LGTM, although I think Ray is a much better person to approve this.

Cool!

In D69074#1713026, @thakis wrote:

This breaks the test on Windows: http://45.33.8.238/win/709/step_9.txt

In D69074#1713028, @thakis wrote:

Now also failing on a more reputable bot: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/28842/steps/test-check-all/logs/stdio