jyu2 committed rGb8fee677bf8e: Re-check in clang support gun asm goto after fixing tests. (authored by jyu2).

jyu2 committed rG954ec09aed4f: clang support gnu asm goto

committed rGc19f4f806972: Fix bug 37903:MS ABI: handle inline static data member and inline variable as… (authored by jyu2).

In D56571#1476266, @nickdesaulniers wrote:

@compudj email me the preprocessed output of basic_percpu_ops_test.c and I'll take a look. (Should be able to find my email via git log).

Hi Reid,
Thanks for your review and catch what I were missing for inline variable part(only inline static data member were fix in previous patch).
As you suggested, instead using GVA_DiscardableODR to put inline variable's initialization function or inline static member's initialization functions into to COMDAT group with global being initialized.

Looks good to me. We are basically de-mangled name for those E initialize global’s function and F destroy global's function.

In D60912#1472987, @rnk wrote:

In D60912#1472986, @jyu2 wrote:

inline static int aoo = foo(); // C++17 inline variable, thus also a definition

This is a static inline global variable, so it technically creates two different globals, so calling foo twice is intended behavior. I think we really want to look at the GVA linkage instead of trying to list all the reasons why something might have weak linkage. Take a look at shouldBeInCOMDAT in CodeGenModule.cpp, I think it has the logic we want.

In D60912#1472983, @rnk wrote:

Thanks, I think there's another case to handle, though.

Closed by commit rG0b28b8b09be7: Bug-40323: MS ABI adding template static member in the linker directive section… (authored by jyu2).

In D58160#1433458, @rnk wrote:

lgtm

Rebased

Rebase

Review comments addressed.

Review comment addressed

Cleanup CFG code, remove obj-lifetime code when build CFE for asm-goto.

I find some ambiguous error for asm-goto out of scope.
Instead call VerifyIndirectOrAsmJumps one time, call that function twice one for indirect-goto one for asm-goto.

Remove check for %lN
Fix missing successor of asm goto in CFG build.

small fix.

More change from review.

1>emit better error for use of output constraints.
2> Add assert for NumLabels and NumOutputs are both larger than 0.

More review comments addressed.

1> Add code for scope checking
2> Using CastInterator
3> CFG change to remove duplicate Block list

Change test for BE IR change.

Change error message.

I add additional test.

Add code to diagnostic error for use of the "l" modifier that does not point to a label in the label list.

Couple of change to respond Eli’s comments.

1> I add code for CFG.cpp and a test for that, as efriedman request.
2> changes are respond the comments I received

In D33537#827509, @baloghadamsoftware wrote:

Test changed. I made some bad throws reachable, but the frontend check still does detects them.

In D33537#801889, @baloghadamsoftware wrote:

In D33537#771274, @aaron.ballman wrote:

The check in D33333 is using a CFG, not just checking direct throws.

I tested the latest revision (the fronted patch already included) on my test file. Disregarding of the not so important parameters (EnabledFunctions and IgnoredExceptions) I do not get warnings for any indirect throws (indirect_implicit() and indirect_explicit()). So for me it does not seem to be using the CFG. Furthermore, I do not get warning for throw_and_catch_some() where 1.1 is a double thus catch(int &) should not catch it. The same happens in throw_catch_rethrow_the_rest(), where catch(int &) should not catch 1.1, but catch(...) should catch and rethrow it. This latter may be a bug.

Hi Richard,

Hi Aaron,
Thank you so much for your review. I just update change to address your comments.

Thank you so much Richard for your review. It is very good to know this select method for diagnostic.

Update test.

update patch

Update patch

Thanks Aaron!!! I just upload new patch to address your comments. I now understand your point on when I can use auto.

Address Aaron's comments.

Add more test include 1> throw/catch reference types. 2> try block. 3>unreachable code.

In D33333#770238, @aaron.ballman wrote:

In D33333#768332, @jyu2 wrote:

Okay this CFG version of this change. In this change I am basic using same algorithm with -Winfinite-recursion.

In addition to my original implementation, I add handler type checking which basic using https://reviews.llvm.org/D19201 method.

Thank you, I think this is a step in the right direction!

There are couple things I am worry about this implementation:
1> compile time...

Do you have any timing data on whether this has a negative performance impact?

2> Correctness...

Your implementation looks reasonable to me, but with further review (and good tests), we should have a better grasp on correctness.

3> Stack overflow for large CFG...

I would be surprised if that were a problem, but is something we could address if it ever arises.

Update to address review comments.

Okay this CFG version of this change. In this change I am basic using same algorithm with -Winfinite-recursion.

I missed two place which Aaron point out.
1> using isNothrow function instead NR_Nothrow.
2> A format problem.

This is new version should address all @Aaron's commands, but CFG part.

In D33333#761233, @aaron.ballman wrote:

In D33333#761224, @rnk wrote:

Re: clang-tidy, I would rather implement this as a traditional compiler warning.

In D33333#761208, @aaron.ballman wrote:

In D33333#761126, @jyu2 wrote:

As I said, I don't think checking throw type matching catch handler type is compiler's job. I'd like either silence all warning for that. What do you think?

Consider the following cases:
...
I think the expected behavior in these cases is reasonable (and can be done with a CFG) rather than manually looking at the scope stack like you're doing.

I agree with @jyu2, we should do something simple. I think it would be simple to handle all cases except for h, which requires semantic analysis to figure out if the thrown object would be caught by the catch parameter. Implementing that is more likely to introduce bugs in the compiler than it is to find bugs in user code.

I'm not certain I agree with the assertion it's more likely to introduce bugs in the compiler than find bugs in user code. Machine-generated code runs into silly things like this, and it's nice to warn the user about it. However, it may be reasonable to do that in a follow-up patch -- but that patch is likely to have a lot of churn since it's not really plausible to implement with the way @jyu2 has structured this patch. By making this an analysis-based warning instead, that would alleviate my concerns (but would likely require implementing the CFG approach anyway).

I doubt we need the CFG for any of this. The later examples are all throwing exceptions from catch blocks, which is the same as not having an open try scope.

Sure, if you cut out the cases that require a CFG, you don't need a CFG for it. :-P Use of a CFG would eliminate obvious false-positives such as h().

Also, there should be some tests with function-try-blocks.

In D33333#760431, @aaron.ballman wrote:

In D33333#760419, @jyu2 wrote:

In D33333#760149, @aaron.ballman wrote:

As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.

To my understanding, clang-tidy is kind of source check tool. It does more intensive checking than compiler.
My purpose here is to emit minimum warning form compiler, in case, clang-tidy is not used.

Sort of correct. clang-tidy doesn't necessarily do more intensive checking than the compiler. It's more an AST matching source checking tool for checks that are either expensive or have a higher false-positive rate than we'd want to see in the frontend.

I think that this particular check should probably be in the frontend, like you're doing. My biggest concern is that we do not duplicate functionality between the two tools. D19201 has has a considerable amount of review, so you should look at the test cases there and ensure you are handling them (including the fixits). Hopefully your patch ends up covering all of the functionality in the clang-tidy patch.

In D33333#760353, @Prazek wrote:

Could you add similar tests as the ones that Stanislaw provied in his patch?
Like the one with checking if throw is catched, or the conditional noexcept (by a macro, etc)

Good idea! Could add “marco” test for this. But I am not sure compiler want to add check for throw caught by different catch handler. Because at compile time, compiler can not statically determine which catch handler will be used to caught the exception on all time. I would think that is pragma's responsibility.

For example:

If (a) throw A else throw B;

My main concern there is implicit noexcept gets set by compiler, which cause runtime to termination.

In D33333#760149, @aaron.ballman wrote:

As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.

Reid,
Thank you so much for your comments. I upload new patch to address your suggestion.
1> Emit warning for throw exception in all noexcept function. And special diagnostic note for destructor and delete operators.
2> Silence this warning when the throw inside try block.

last one missing test part diff. Sorry for that