In D72522#1819964, @sidneym wrote:

Update testcase.

• Verify the absence of relocations in the executable.
• hexagon may call tls_get_addr but the name is not explicitly stated in the ABI. The internally maintained linker will create the symbol to call which is generally "tls_get_addr". There are instances when it would not be "tls_get_addr" such as when a user built a static executable using objects built -shared -pic. In such a case the internal linker would create a stub called __hexagon_ie_tls_get_addr and call it. LLD for hexagon would not support doing this until some design could be agreed upon.
• Don't check explicit addresses in GOT.
• Added the check for the closing brace.

This patch has not been composed correctly, it accidentally adds test/ content to the top of the tree.

LGTM. @kparzysz what do you think?

LGTM

LGTM but you should probably get approval from a reviewer w/experience in MCSubtargetInfo.h.

Is this a permanent constraint for AMDGPU or just a temporary workaround? If the latter, it's best to annotate that's the case.

In D69192#1721507, @MaskRay wrote:

Thanks for the update. sh_offset > file size is what GNU objcopy may do. LG but please wait for others' opinions.

Shouldn't the first patch add to LLVM_TARGETS_TO_BUILD or LLVM_EXPERIMENTAL_TARGETS_TO_BUILD?

LGTM

LGTM, but suggestions for stricter test case.

LGTM

In D68741#1703014, @ro wrote:

Is there any point doing this now when the switch to git is imminent?

In D66542#1640502, @MaskRay wrote:

I sent a request to download hexagon-application-binary-interface-specification.zip a few days ago but I still don't get a copy...

The only information in the Internet about R_HEX_B15_PCREL_X is probably this patch now.. I'd say this is very inconvenient for anyone who'd like to contribute to verify the correctness. Since there is not any reference available, I can only guess.

What do the _X relocation types represent? Why are some R_PLT_PC while the others are R_PC?

This patch is intended to address new warnings like the one below that shows up with clang on a hexagon target:

This will appear as a 9.0 regression, right? Let's please consider a way to address it for 9.0 before it's released.

In D38630#1569171, @mclow.lists wrote:

I think this patch is dead; the problem has been solved using _LIBCPP_UNUSED_VAR Is that correct?

Obsoleted by r364039.

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