Ping.

Rebase, and add a unit test to demonstrate the bug.

This incorporates all the improvements I think we've discussed so far:

@jdoerfert Thanks for your reply. I added some comments to the tests. Let me know whether that's what you're looking for.

Simplify test as discussed with @probinson. Improve wording in docs slightly.

In D64135#1569395, @probinson wrote:

lit has options?

In D64135#1569174, @rnk wrote:

I think it would be preferable to standardize on llvm-lit as the way to re-run particular tests with some options.

Fix minor problems in the summary and a test.

Rebased, fixed a comment, and applied a clang-format suggestion.

Ping.

Never mind. I see it now. LGTM.

This looks equivalent to me.

Ping.

In D61509#1512601, @Meinersbur wrote:

I would think different people would want to review different pragmas, so separate patches would be better, but I'm happy to be corrected as I haven't explored who owns what here.

AFICS it is changing Tok.setLocation(FirstTok.getLocation()); to Tok.setLocation(Introducer.Loc); for most PragmaHandlers that emit an annotation token.

In D61509#1512570, @aaron.ballman wrote:

These changes LGTM!

In D61509#1512488, @aaron.ballman wrote:

In D61509#1512090, @jdenny wrote:

Now that D61643 is pushed, we're back to this patch. My recollection is there are two remaining issues:

1. Should we store both the #pragma location and the omp location in the AST, or is it fine to just replace the latter location with the former? If we choose to store both, we still haven't settled on an implementation, which likely will involve non-trivial changes in the parser and the AST. For OpenMP, @ABataev said replacing should be fine.

Storing both would be nice, but not required, IMO. Storing the location of the pragma "namespace" would be useful for third-party tooling purposes, but I don't think we have a need for it in the frontend otherwise.

In D61509#1512321, @Meinersbur wrote:

In D61509#1512311, @jdenny wrote:

2. I too think it likely makes sense to adjust them all eventually. But do people think it's important to write patches adjusting all pragmas before pushing the adjustment for any of them?

I am not sure I understand. Do you mean whether you need all patches for each pragma to be accepted before you can commit the first? This is not that case.

I tried at r361335, where the original reproducer failed this assert consistently quickly for me today. After applying your patch there and running each reproducer from my bug report in a shell while loop for about an hour, I saw no failures. Thanks!

In D61509#1512294, @ABataev wrote:

In D61509#1512293, @Meinersbur wrote:

1. Is there a diagnostic that would point to the omp token? As much as I like complete info (such as SourceLoc of semicolons), I cannot think of a use case for it.
2. I would like to see all of them all adjusted. There is an immediate improvement in that improves the diagnostic output.

1. Nope, there is no omp specific diagnostics

Now that D61643 is pushed, we're back to this patch. My recollection is there are two remaining issues:

In D61643#1510815, @Meinersbur wrote:

+1

Such a solution also came up in https://bugs.llvm.org/show_bug.cgi?id=41514#c1

In D61944#1503254, @AndreyChurbanov wrote:

Thanks, Joel.

I actually was not able to reproduce your first problem. But I hope the patch should fix both.

For me, this appears to fix my bug report's second reproducer, the one without the num_teams(3). Thanks!

Ping. This seems like a straight-forward extension to the Rewriter API.

As requested, replace this patch with just the OpenMP changes.

In D61509#1493729, @lebedev.ri wrote:

I don't see why this differential can't be updated to only contain the remaining part
of the diff (for the actual OpenMP change), after splitting the NFC refactoring part.

In D61509#1493718, @lebedev.ri wrote:

In D61509#1493714, @jdenny wrote:

In D61509#1493703, @lebedev.ri wrote:

It would have been better to submit this refactor as a new patch..

Sorry, I didn't realize that was the norm. I can do that now if it would help. I can also revert changes to this patch if the goal is to make it easier to reference the old version.

I think that would be good. This current diff would be a simple NFC cleanup, i think i will signoff it even.

In D61509#1493703, @lebedev.ri wrote:

It would have been better to submit this refactor as a new patch..

As suggested, I've created a struct PragmaIntroducer and I've reduced this patch not to include the OpenMP changes. I'll add a new patch with the OpenMP changes soon.

LGTM except for nits in the tests. I'm not close to C++ support in Clang, so please give other reviewers a few days to comment just in case. I've added a couple who have reviewed my patches in this area in the past.

Thanks for everyone's responses so far.

In D61509#1491898, @ABataev wrote:

Again, I don't see a single point why would we need this. I don't think there is a big difference between the location of pragma keyword and omp keyword.

In D61509#1491537, @lebedev.ri wrote:

In D61509#1491397, @jdenny wrote:

In D61509#1491209, @ABataev wrote:

In D61509#1491204, @lebedev.ri wrote:

In D61509#1491158, @jdenny wrote:

In D61509#1491119, @lebedev.ri wrote:

I recommend to split this into two parts - changing PragmaIntroducerKind Introducer to
struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};, and the actual openmp change.

Sure, I'll work on that. What about NameMe = PragmaIntroducer?

Could work. And then move PragmaIntroducerKind into it.
I believe this part of the refactoring is completely uncontroversial.

For that change, just basing off the clang-tidy diff, neither variant is ideal,

Do you mean that it's better for diagnostics that point to a pragma not to include the #pragma in their locations? If so, why is that?

I'm not sure either one is better than the other one.

I have two concerns:

• I fear this would result in inconsistency with other pragmas, since this will only change openmp-ones. I don't know if it will be accepted to migrate the rest of them in the same way.
• This use-case requires having the location of #pragma, so the entire AST is migrating to store it. But the current location will no longer be accessible from AST.
  
  I see two paths forward:
• Mail cfe-dev, and suggest to do this change for *all* pragmas. Either this is ok for all of them, or none of them.
• Moar abstractions - how about not changing the startloc of openmp directives,

My alternative proposal was exactly that. A difficulty is how to pass the #pragma location to the OpenMP AST node constructors. PragmaOpenMPHandler::HandlePragma passes locations via the tok::annot_pragma_openmp and tok::annot_pragma_openmp_end tokens, so where do we pass this new location? I proposed creating a third token, and Alexey was concerned over the parsing problems that would create.

but instead add some baseclass to `OMPDirective` class (& every other class that is created from pragma), that would record the `PragmaIntroducer`?

I agree that a base class would be a nice way to extend all pragma classes with the PragmaIntroducer.

I'm against this solution. I don't see any reasons why we should do this. Instead, we're getting a lot of pain with parsing and maintenance.

One way to avoid creating an extra token would be to widen the Token class to store the additional location. The Token documentation says it's not intended to be space efficient. How does that sound to people?

That was my proposal, yes.

In D61509#1491209, @ABataev wrote:

In D61509#1491204, @lebedev.ri wrote:

In D61509#1491158, @jdenny wrote:

In D61509#1491119, @lebedev.ri wrote:

I recommend to split this into two parts - changing PragmaIntroducerKind Introducer to
struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};, and the actual openmp change.

Sure, I'll work on that. What about NameMe = PragmaIntroducer?

Could work. And then move PragmaIntroducerKind into it.
I believe this part of the refactoring is completely uncontroversial.

For that change, just basing off the clang-tidy diff, neither variant is ideal,

Do you mean that it's better for diagnostics that point to a pragma not to include the #pragma in their locations? If so, why is that?

I'm not sure either one is better than the other one.

I have two concerns:

• I fear this would result in inconsistency with other pragmas, since this will only change openmp-ones. I don't know if it will be accepted to migrate the rest of them in the same way.
• This use-case requires having the location of #pragma, so the entire AST is migrating to store it. But the current location will no longer be accessible from AST.
  
  I see two paths forward:
• Mail cfe-dev, and suggest to do this change for *all* pragmas. Either this is ok for all of them, or none of them.
• Moar abstractions - how about not changing the startloc of openmp directives,

In D61509#1491119, @lebedev.ri wrote:

I recommend to split this into two parts - changing PragmaIntroducerKind Introducer to
struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};, and the actual openmp change.

As discussed, implement OpenMP solution #1 (one-line change in PragmaOpenMPHandler::HandlePragma in ParsePragma.cpp), and update tests.

Thanks for explaining. I'll proceed with solution #1.

In D61509#1489771, @ABataev wrote:

In D61509#1489761, @jdenny wrote:

In D61509#1489752, @ABataev wrote:

If the patch is going to be accepted, then definitely it must be solution #1.

I certainly have no objection to that and will be happy to implement it, but can you help me to understand the rationale? (Thanks for your quick response!)

Another one annotation token may significantly change the parsing process. It will require a lot of rework in the parsing of OpenMP pragmas plus may lead to some unpredictable results like endless parsing in some cases etc.

In D61509#1489752, @ABataev wrote:

If the patch is going to be accepted, then definitely it must be solution #1.

In D59168#1484754, @phosek wrote:

It's the LIBCXX_ABI_VERSION CMake option, see https://github.com/llvm/llvm-project/blob/master/libcxx/CMakeLists.txt#L121

In D59168#1480801, @phosek wrote:

In D59168#1480428, @jdenny wrote:

It seems we have roughly the same situation for their ABIs. That is, for .so files, someone noted that libc++ and libunwind have always used the same version, .1, and openmp doesn't use a version (except in Debian packages). In other words, for each of these three, a change in ABI compatibility has never been indicated. For openmp only, it's apparently assumed a change never will need to be indicated (except in Debian packages).

They already use .2 for ABI v2 which is currently considered unstable but some project like Fuchsia already use. There's also an ongoing discussion to stabilize v2.

In D59168#1479939, @phosek wrote:

In D59168#1474715, @jdenny wrote:

Does the following match what you have in mind?

$prefix/
  include/
    c++/
      v1/
        limits.h
        ...
    openmp/
      v1/ <------ I don't think openmp has anything like this now

Iibc++ uses this scheme to allow the possibility of evolving the API but I'm not sure if it's necessary for openmp.