Page MenuHomePhabricator

[CTU] Do not allow different CPP dialects in CTU
ClosedPublic

Authored by martong on Feb 7 2019, 9:52 AM.

Details

Summary

If CPP dialects are different then return with error.

Consider this STL code:

template<typename _Alloc>
  struct __alloc_traits
#if __cplusplus >= 201103L
  : std::allocator_traits<_Alloc>
#endif
  { // ...
  };

This class template would create ODR errors during merging the two units,
since in one translation unit the class template has a base class, however
in the other unit it has none.

Diff Detail

Repository
rC Clang

Event Timeline

martong created this revision.Feb 7 2019, 9:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 9:52 AM

Hi Gabor,
Please find my comments inline.

lib/CrossTU/CrossTranslationUnit.cpp
237

The comment change looks strange.

259

Should we create another counter, like NumLangDialectMismatch?

martong updated this revision to Diff 185926.Feb 8 2019, 1:32 AM
martong marked 4 inline comments as done.
  • Revert comment change
  • Add lang_dialect_mismatch and stats for that
martong added inline comments.Feb 8 2019, 1:33 AM
lib/CrossTU/CrossTranslationUnit.cpp
237

Ok, I reverted this hunk.

259

Okay, I added the new error code lang_dialect_mismatch too.

Consider this STL code:

template<typename _Alloc>
  struct __alloc_traits
#if __cplusplus >= 201103L
  : std::allocator_traits<_Alloc>
#endif
  { // ...
  };

This class template would create ODR errors during merging the two units,
since in one translation unit the class template has a base class, however
in the other unit it has none.

How is #if __cplusplus >= 201103L qualitatively different from #ifndef NDEBUG or #if MYLIB_ABI_VERSION==2 or #if __DATE__ == "2018-04-01"?

r.stahl added inline comments.Feb 8 2019, 7:33 AM
lib/CrossTU/CrossTranslationUnit.cpp
255

This is likely to become a bug in the future, but I didn't see a better way to compare dialects.

Is there a way to add a test that lights up once there is a new dialect?

Would it be too pessimistic to compare the entire LangOpts? Some stuff in there should even still produce errors, right? For example "GnuMode". I skimmed over them and didn't find an obvious one that would differ between translation units of the same project. Maybe the template depth could be set selectively, but to fix that we could mask "COMPATIBLE_" and "BENIGN_" opts.

How is #if cplusplus >= 201103L qualitatively different from #ifndef NDEBUG or #if MYLIB_ABI_VERSION==2 or #if DATE__ == "2018-04-01"?

Ideally, all macros should be the same in the two TUs... If we were very strict then we could check for that, but that might be just too strict.

If there is an ODR error (either because of a macro mismatch or for other reasons) then structural equivalency will diagnose that, but may be nontrivial for the user to identify what causes the ODR error.
Checking the language and the language dialect is easy and it prevents such disturbing errors early.

martong marked 2 inline comments as done.Feb 26 2019, 9:29 AM
martong added inline comments.
lib/CrossTU/CrossTranslationUnit.cpp
255

This is likely to become a bug in the future, but I didn't see a better way to compare dialects.
Is there a way to add a test that lights up once there is a new dialect?

LANGOPTS are defined as bitfields: #define LANGOPT(Name, Bits, Default, Description) unsigned Name : Bits;
I can't think of any solution to avoid the future bug, because there is no way to enumerate all the C++ dialects.
I am going to ask around about this on cfe-dev, maybe somebody will have an idea.

Would it be too pessimistic to compare the entire LangOpts?

I think that would be too pessimistic.

xazax.hun accepted this revision.Feb 28 2019, 7:07 AM

LGTM! I think we should commit this as is for now but maybe adding a TODO comment to summarize the problem would be nice. Maybe we could have an isSameDialect or similar method within LangOpts, so it is harder to break this code.

This revision is now accepted and ready to land.Feb 28 2019, 7:07 AM
This revision was automatically updated to reflect the committed changes.
martong marked an inline comment as done.