In D54348#1531741, @cmtice wrote:

Ping? Is anybody looking at or working on this? The bug this fixes is one of the issues currently blocking Chrome OS from being able to adopt Dwarf v5, so we are hoping for a fix?

In D61915#1503525, @ldionne wrote:

In D61915#1503235, @bcain wrote:

Maybe the presence of the filesystem header is enough to get some of these tests to pass. [...]

Yes, I think this is exactly what's happening, and this is what I wanted to understand. I think the best solution would be for LIBCXX_ENABLE_FILESYSTEM to actually control whether filesystem is enabled in the library, however this includes:

• whether we build support for filesystem in the dylib (that's the only thing we already do)
• whether the <filesystem> header is provided
• whether we provide operations like std::ifstream::open() on std::filesystem::path & friends
  
  I think if we had that, then we could rename the lit feature from dylib-has-no-filesystem to just has-no-filesystem, and that feature would be enabled whenever:
• we are testing back-deployment to a platform that doesn't have dylib support for filesystem (and has availability markup that makes it invalid to use stuff like std::ifstream::open(std::filesystem::path))
• we are testing libc++ trunk without filesystem support (your use case)
  
  Is this something you'd be willing to investigate? I think having a good example of how to disable complete features from the library would be useful for other things as well, so this is something we need to do at some point. For example, our story for things like LIBCXX_ENABLE_STDIN and LIBCXX_ENABLE_STDOUT is similarly broken.

In D61915#1503235, @bcain wrote:

...

Maybe the presence of the filesystem header is enough to get some of these tests to pass. So maybe I should go ahead and change this patch to do that instead?

In D61915#1503179, @mclow.lists wrote:

In D61915#1502132, @bcain wrote:

...

(I'm going to comment on just the variant/optional/ any tests here, because I think they're straightforward).
They're marked as XFAIL when the dylib doesn't have the appropriate exception classes - and they should fail.
They also should fail if they are built with exceptions disabled - because the exceptions will never get thrown. This behavior should be confirmed.

In D61915#1502008, @ldionne wrote:

I'd like to better understand what problem this is solving:

• What platform are you testing on?
• How did you configure CMake?
• How do you run lit?

Previous patch was missing update to config.py.

Changed to use 'has-fork' feature instead of 'debug-mode'.

In D61348#1486428, @bcain wrote:

Changed debug-mode feature to be deactivated when the LIBCXX_TEST_HAVE_FORK option is disabled. This hopefully sidesteps the debate triggered by the previous revision.

In D61655#1494372, @EricWF wrote:

This LGTM.

Although wouldn't unsigned int be the correct choice on all platforms? Because the issue only arises because sometimes uint32_t is unsigned long instead of unsigned int?

More context for this change:

ping! any comments or concerns about this change?

In D61403#1486958, @ldionne wrote:

I have a workaround for this locally. I guess this is only relevant with older CMakes.

Changed debug-mode feature to be deactivated when the LIBCXX_TEST_HAVE_FORK option is disabled. This hopefully sidesteps the debate triggered by the previous revision.

In D61348#1486014, @mclow.lists wrote:

In D61348#1485462, @EricWF wrote:

These tests turn on debug mode themselves - they don't need to be limited the configurations where the user turns it on.

These tests need to be fixed so that they DTRT when the user specifies -debug_level=X (where X is 0/1) as a LIT param as well.

ping? any comments/concerns?

Add context, add clang update.

Added context lines.

In D58091#1397586, @jyknight wrote:

I think this warning (-Wbuiltin-requires-header) doesn't really make sense as its own warning.

We already have two related (on-by-default) warnings.

...

In D58091#1409584, @jdoerfert wrote:

I did address the comments but I will wait until I hear back on the "warning vs warning + note question".

...

I do not have strong feelings about this, either way is fine with me. However, I lack the
clang expertise to make such a change happen anytime soon which makes this patch
(with actual fix for the warning on pthread_create) my prefered first step.

I can confirm that this fix is effective at addressing the problem we experienced.

Are you sure it is unrelated?

I reported PR40692. I just tried this patch on our local build where we saw the failure on an RTOS implementing pthreads. Unfortunately with this patch I encountered an (unrelated) assertion. So this fix was inconclusive for me (for now). I will follow up but if this fix makes sense then you don't need to wait for my test results.

In D54348#1295895, @dblaikie wrote:

...

So probably need to have some discussion about when/where/if this should fire.

The directive_file-4.s test passes and no existing tests fail with this change.

Updated per review, test-release.sh build completes w/this patch on sles11.3.

In D51315#1214857, @probinson wrote:

The indentation in the test looks a little odd. Please verify there are no hard tabs.
Otherwise LGTM.

Please cite the PR number in the commit message, when you push this.

• Changed indentation for nop
• Added PR number to summary

Comments:

I think this is now obsolete due to clang changing the warning.

r339981

Should we have this build configuration in a buildbot?

LGTM

LGTM

FYI several of the issues that I found are ones that I discovered when I tried to build the patch with 6.0.rc2. Also, I didn't get to detail them here but there were warnings regarding virtual methods w/o virtual destructor. I believe those were on the classes inherited from HWEventListener.

Thanks for the contribution, it looks interesting.

Changed per review

In D41727#979404, @EricWF wrote:

LGTM with the change mentioned by @lebedev.ri .

this was committed by Marshall

In D41727#967371, @lebedev.ri wrote:

Note that it won't help for the cases where libc++ is used, and -Wextra -Wsystem-headers is specified.

In D39462#959822, @phosek wrote:

FWIW we've already rolled Clang that contains -Wtautological-constant-compare to our codebase and we had to set -Wno-tautological-constant-compare globally because we were getting bogus warnings in many places, similarly to D41368. If that warning ships as part of Clang 6, it's going to be be a major pain for many users so we really need some solution.

In D41368#959579, @smeenai wrote:

@mclow.lists are you okay with this approach? I'm also fine using a cast to silence the warning, as @zturner suggested, but we should suppress the warning in some way, otherwise libc++ 6 is gonna have compile warnings with clang 6 out of the box, which isn't great.

A third alternative, which is the least invasive, though not complete in some sense: we just add -Wno-tautological-constant-compare to the compile flags for libc++ (in CMake), to suppress the warning during libc++'s compilation. There's still an instance of the warning in a header, but all other clients of the header should treat it as a system header (in which case warnings will be suppressed anyway). It's not targeted at all and could suppress legitimate instances of the warning though.

In D39149#912260, @smeenai wrote:

I confirmed that these warnings go away with D39462 applied, and they reappear if I manually specify -Wmaybe-tautological-constant-compare. Thank you!

In D41316#957598, @jroelofs wrote:

I'd much rather provide no implementation than one that lies. Broken builds are much safer than problems at runtime.

I think some of the XFAIL: availability may be wrong here. I'd submit a patch, but it's not clear to me what the appropriate fix is.

I'd like to understand/resurrect this change, so I'll try to summarize. Please correct this as appropriate:

I think it's a mistake to have the behavior be contingent on available disk space for the destination partition. Has that proposal been ruled out now?

In D38635#917435, @EricWF wrote:

Are the changes that actually use this feature going to be upstreamed as well? Or are they specific to an internal build? It's never great to introduce seemingly dead or unused code.

In D39149#910931, @lebedev.ri wrote:

In D39149#910891, @bcain wrote:

In D39149#910845, @lebedev.ri wrote:

That is my diagnostic, so i guess this is the time to reply :)

...

3. In -Wtautological-constant-compare, ignore any comparisons that compare with std::numeric_limits. Not a fan of this solution. <- Worst?

...

Gee, #3 is the worst? That one seems the most appealing to me. I think I could understand it if you had reservations about ignoring comparisons against {CHAR,SHRT,INT,LONG}_{MIN,MAX} but std::numeric_limits is well-qualified and everything.

Can you help me understand why you don't prefer it?

That my initial bug was not caught by gcc either, even though -Wtype-limits was enabled.

...

In D39149#910845, @lebedev.ri wrote:

That is my diagnostic, so i guess this is the time to reply :)

...

Made the recommended changes.

Updated per review comments.

In D35802#857702, @mclow.lists wrote:

Can you give an example of linux distro names with spaces in it?
Other than that, this looks fine to me.