Page MenuHomePhabricator

[clang] Include clang config.h in LangStandards.cpp
ClosedPublic

Authored by porglezomp on Wed, May 4, 7:14 PM.

Details

Summary

This is necessary in order to pick up the default C/C++ standard from
the CLANG_DEFAULT_STD_C(XX) defines. This fixes a bug that was
introduced when this default language standard code was moved from
Frontend to Basic, making compilers ignore the configured default
language version override.

Fixes a bug introduced by D121375.

Diff Detail

Event Timeline

porglezomp created this revision.Wed, May 4, 7:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, May 4, 7:14 PM
porglezomp requested review of this revision.Wed, May 4, 7:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, May 4, 7:14 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
hokein accepted this revision.Thu, May 5, 12:32 AM

Thanks!

This revision is now accepted and ready to land.Thu, May 5, 12:32 AM

Can we add a test for this?

Can we add a test for this?

I think the problem with testing it is that this variable comes from the CMake config, and in the default configuration (as used by all the bots) it's not set.

I would be interested in adding some kind of test for this, but am not sure how.

I think the robust general-purpose check here would be to ensure that no file is referring to symbols defined by the various config.h etc. files without including those files, but am not sure how to do that analysis, or if it's feasible with legacy code. That's obviously a much larger thing to test than this one change, and might run into problems if there are any define flags that don't come from the CMake config.

Unless anyone has an idea about how to test this, I'm going to commit this without adding a test.

Can we add a test for this?

I think the problem with testing it is that this variable comes from the CMake config, and in the default configuration (as used by all the bots) it's not set.

Maybe you could use the same CMake config to set a lit variable, and use that lit variable to test that we get the configured default language variant.

Ah, so it'd be a test that passes pretty trivially on any bot that doesn't have a custom version of CLANG_DEFAULT_STD_C and CLANG_DEFAULT_STD_CXX like the default config, but could get caught by any other bots that do set it?

What's the best way to get the compiler to report the standard it's using for this test?

For C++ I can do:

clang++ -xc++ -dM -E - < /dev/null | grep cplusplus

And it will print e.g.

#define __cplusplus 199711L

But that requires quite some decoding to compare against CLANG_DEFAULT_STD_CXX which would look like LangStandard::lang_gnucxx98.

Ah, so it'd be a test that passes pretty trivially on any bot that doesn't have a custom version of CLANG_DEFAULT_STD_C and CLANG_DEFAULT_STD_CXX like the default config, but could get caught by any other bots that do set it?

What's the best way to get the compiler to report the standard it's using for this test?

For C++ I can do:

clang++ -xc++ -dM -E - < /dev/null | grep cplusplus

And it will print e.g.

#define __cplusplus 199711L

But that requires quite some decoding to compare against CLANG_DEFAULT_STD_CXX which would look like LangStandard::lang_gnucxx98.

Yeah, that seems awkward.

My concern with lacking a test is that someone could easily remove this in the future when trying to prune unused #includes.

Another solution might be to change this to a compile error. I think it'd work to change the #cmakedefine logic to:

#cmakedefine CLANG_DEFAULT_STD_C LangStandard::lang_${CLANG_DEFAULT_STD_C}
// Always #define something so that missing the config.h #include at use sites
// becomes a compile error.
#ifndef CLANG_DEFAULT_STD_C
#define CLANG_DEFAULT_STD_C LangStandard::lang_unknown
#endif

and then update the code to:

case Language::C:
  if (CLANG_DEFAULT_STD_C != LangStandard::lang_unknown)
    return CLANG_DEFAULT_STD_C;

  // The PS4 uses C99 as the default C standard.
  if (T.isPS4())
    return LangStandard::lang_gnu99;
  return LangStandard::lang_gnu17;

(without the #ifdef)

WDYT?

That looks good to me. I'm testing it out locally to make sure it still works right and also catches the issue, and will plan put up an updated patch in the morning.

Add changes suggested by Duncan to make CLANG_DEFAULT_STD_C more misuse-resistant

Arcanist accidentally threw away the first commit in the sequence...

I haven't used arc in a little while and am having trouble sorry for the emails

dexonsmith accepted this revision.Fri, May 13, 11:21 PM

LGTM, thanks! One comment online below.

clang/include/clang/Config/config.h.cmake
19 ↗(On Diff #429275)

I just noticed that the other comments in this file are C-style. Probably the new ones should be too.