Page MenuHomePhabricator

[ASTImporter] Make ODR diagnostics warning by default
AbandonedPublic

Authored by gamesh411 on Dec 13 2018, 1:33 AM.

Details

Summary
  • Make ODR diagnostics warning by default

After this change all odr-related diagnostics are considered warnings belonging
to Diagnostic Group "OneDefinitionRule". They can be propagated to error level
with '-Werror=odr'.

  • Remove unused ErrorOnTagTypeMismatch member

Diff Detail

Event Timeline

gamesh411 created this revision.Dec 13 2018, 1:33 AM

In order to be able to handle ODR-related diagnostics with command line options, these diagnostics were moved from Error category to Warning. What are your thoughts?

The primary reason why want these to be Warnings instead of Errors because there are many false positive ODR Errors at the moment. These are not fatal errors but there is a maximum error count which once reached the whole process (analysis) is aborted.
Usually, such false positives are related to a wrong import of redeclaration chains or existing errors in the structural match logic.

Hello Endre.
I agree that it doesn't make sense to have 'errors' in AST merging tools, and the changes for ASTImporter part are welcome. However, I don't feel so positive to modules support changes and I don't think we are allowed to change this. @aaron.ballman, @rsmith what do you think?
@bruno, you're the original author of the related Sema code. What do you think, should this be a warning or an error in Sema?

Hey Aleksei!
Thank you for the input! ODR violations being warnings would be beneficial from the code maintenance point of view, as we would not have to resort to duplicate some (if not most) of them as errors. There is also a flexibility advantage in the diagnostics, as warnings can be propagated to error level or suppressed, whereas errors cannot be "demoted" (AFAIK). So while I am not opposed to providing an error AND a warning for these kinds of violations, if the SEMA or other modules do not need them to be explicitly defined as errors (for I don't know... tooling support or other reasons), then it would be cleaner to only have warnings for these.

Just a quick note. We are pretty sure that StructuralEquivalency can have false positive results, i.e. it can report two decls as nonequivalent falsely. My understanding is that we should report an error only if we are absolutely certain that an error has happened, this is not the case with StructuralEquivalency. Consequently this should be a warning in Sema (modules) too.

In general, I agree that it doesn't make sense for ODR violations (with false positives) to trigger error diagnostics while performing AST merging, so downgrading this to warnings is a good idea. However, I do worry about impacting modules and C compatibility with the changes. I'm curious what @rsmith thinks the answer should be here. AFAIK, modules do not *require* ODR violations checks (I believe that's still ill-formed, no diagnostic required), but confirmation would be good.

lib/Sema/SemaType.cpp
7609

Are we sure this isn't breaking C compatibility? IIRC, C has structural equivalence rules because it doesn't have quite the same notion of an ODR as C++ does regarding things like tag redeclarations. I worry that we'll be downgrading valid errors into warnings here because of that.

I am going to abandon this modification, as setting ODR violations as warnings, seems like a change that could have unforeseen consequences.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2019, 2:22 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript
gamesh411 abandoned this revision.Feb 14 2019, 2:28 AM

I am creating a new revision that keeps the old handling of ODR violations the same when used by parts of the Sema, but emit warnings when used by the ASTImporter.