r258504 - Change of UserLabelPrefix default value from "_" to ""
Andrey Bokhanko via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 27 06:38:22 PST 2016
Yes, I understand this -- but the whole idea of my patch is to change
the blanket default, on the whole table.
Rafael believes this is the right thing to do (actually, he is the one
who requested the change), and judging by feedback on PR26255, he is
right.
Please let me know when you'll add the tests for targets not yet
covered, as I'd like to fix them and get my patch back.
Yours,
Andrey
On Wed, Jan 27, 2016 at 4:42 PM, James Y Knight <jyknight at google.com> wrote:
> Imagine there's a 2d table of values for UserLabelPrefix, each row for CPU
> and column for OS. The value of many if those cells was changed by this
> commit, because you stopped painting columns as "".
>
> That is, originally, the default entry was "_", then cpu rows were filled,
> and then the os columns were painted on top of that.
>
> Now, the Sparc row gets painted "_", but the rtems column isn't painted on
> top anymore. So, Sparc+rtems gets the wrong value.
>
> As to test cases, yes, I agree, I will add some more preprocessor tests for
> other OS/CPU combinations.
>
> On Jan 27, 2016 3:12 AM, "Andrey Bokhanko" <andreybokhanko at gmail.com> wrote:
>>
>> Hi James,
>>
>> > I reverted this change with r258894, as it breaks (at least)
>> > sparc-rtems.
>>
>> Can you send me a reproducer, please?
>>
>> Right now I'm at a dead end -- no tests are broken, and no buildbots
>> are reported any fails. Yet you claim that my patch breaks one target
>> -- and there is zero info how to verify this.
>>
>> > Clearly this area of the code was not sufficiently covered by the
>> > testsuite.
>>
>> Not sure I understand what you mean -- it's actually covered quite
>> extensively in test/Preprocessor/init.c. There is a line for generic
>> SPARC as well:
>>
>> // SPARC:#define __USER_LABEL_PREFIX__ _
>>
>> and I added initialization of UserLabelPrefix to "_" in
>> SparcTargetInfo constructor specifically to accommodate this test.
>>
>> Perhaps you might want to add tests for sparc-rterms target as well?
>>
>> Yours,
>> Andrey
>>
>>
>> On Wed, Jan 27, 2016 at 4:10 AM, James Y Knight <jyknight at google.com>
>> wrote:
>> > I reverted this change with r258894, as it breaks (at least)
>> > sparc-rtems.
>> > Clearly this area of the code was not sufficiently covered by the
>> > testsuite.
>> >
>> > On Fri, Jan 22, 2016 at 10:24 AM, Andrey Bokhanko via cfe-commits
>> > <cfe-commits at lists.llvm.org> wrote:
>> >>
>> >> Author: asbokhan
>> >> Date: Fri Jan 22 09:24:34 2016
>> >> New Revision: 258504
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=258504&view=rev
>> >> Log:
>> >> Change of UserLabelPrefix default value from "_" to ""
>> >>
>> >> Differential Revision: http://reviews.llvm.org/D16295
>> >>
>> >> Modified:
>> >> cfe/trunk/lib/Basic/TargetInfo.cpp
>> >> cfe/trunk/lib/Basic/Targets.cpp
>> >>
>> >> Modified: cfe/trunk/lib/Basic/TargetInfo.cpp
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/TargetInfo.cpp?rev=258504&r1=258503&r2=258504&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- cfe/trunk/lib/Basic/TargetInfo.cpp (original)
>> >> +++ cfe/trunk/lib/Basic/TargetInfo.cpp Fri Jan 22 09:24:34 2016
>> >> @@ -72,7 +72,7 @@ TargetInfo::TargetInfo(const llvm::Tripl
>> >> DoubleFormat = &llvm::APFloat::IEEEdouble;
>> >> LongDoubleFormat = &llvm::APFloat::IEEEdouble;
>> >> DataLayoutString = nullptr;
>> >> - UserLabelPrefix = "_";
>> >> + UserLabelPrefix = "";
>> >> MCountName = "mcount";
>> >> RegParmMax = 0;
>> >> SSERegParmMax = 0;
>> >>
>> >> Modified: cfe/trunk/lib/Basic/Targets.cpp
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets.cpp?rev=258504&r1=258503&r2=258504&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- cfe/trunk/lib/Basic/Targets.cpp (original)
>> >> +++ cfe/trunk/lib/Basic/Targets.cpp Fri Jan 22 09:24:34 2016
>> >> @@ -102,9 +102,7 @@ protected:
>> >>
>> >> public:
>> >> CloudABITargetInfo(const llvm::Triple &Triple)
>> >> - : OSTargetInfo<Target>(Triple) {
>> >> - this->UserLabelPrefix = "";
>> >> - }
>> >> + : OSTargetInfo<Target>(Triple) {}
>> >> };
>> >>
>> >> static void getDarwinDefines(MacroBuilder &Builder, const LangOptions
>> >> &Opts,
>> >> @@ -242,6 +240,7 @@ public:
>> >> this->TLSSupported = !Triple.isOSVersionLT(2);
>> >>
>> >> this->MCountName = "\01mcount";
>> >> + this->UserLabelPrefix = "_";
>> >> }
>> >>
>> >> std::string isValidSectionSpecifier(StringRef SR) const override {
>> >> @@ -284,8 +283,6 @@ protected:
>> >> public:
>> >> DragonFlyBSDTargetInfo(const llvm::Triple &Triple)
>> >> : OSTargetInfo<Target>(Triple) {
>> >> - this->UserLabelPrefix = "";
>> >> -
>> >> switch (Triple.getArch()) {
>> >> default:
>> >> case llvm::Triple::x86:
>> >> @@ -327,8 +324,6 @@ protected:
>> >> }
>> >> public:
>> >> FreeBSDTargetInfo(const llvm::Triple &Triple) :
>> >> OSTargetInfo<Target>(Triple) {
>> >> - this->UserLabelPrefix = "";
>> >> -
>> >> switch (Triple.getArch()) {
>> >> default:
>> >> case llvm::Triple::x86:
>> >> @@ -368,9 +363,7 @@ protected:
>> >> }
>> >> public:
>> >> KFreeBSDTargetInfo(const llvm::Triple &Triple)
>> >> - : OSTargetInfo<Target>(Triple) {
>> >> - this->UserLabelPrefix = "";
>> >> - }
>> >> + : OSTargetInfo<Target>(Triple) {}
>> >> };
>> >>
>> >> // Minix Target
>> >> @@ -392,9 +385,7 @@ protected:
>> >> DefineStd(Builder, "unix", Opts);
>> >> }
>> >> public:
>> >> - MinixTargetInfo(const llvm::Triple &Triple) :
>> >> OSTargetInfo<Target>(Triple) {
>> >> - this->UserLabelPrefix = "";
>> >> - }
>> >> + MinixTargetInfo(const llvm::Triple &Triple) :
>> >> OSTargetInfo<Target>(Triple) {}
>> >> };
>> >>
>> >> // Linux target
>> >> @@ -467,7 +458,6 @@ protected:
>> >> }
>> >> public:
>> >> NetBSDTargetInfo(const llvm::Triple &Triple) :
>> >> OSTargetInfo<Target>(Triple) {
>> >> - this->UserLabelPrefix = "";
>> >> this->MCountName = "_mcount";
>> >> }
>> >> };
>> >> @@ -488,7 +478,6 @@ protected:
>> >> }
>> >> public:
>> >> OpenBSDTargetInfo(const llvm::Triple &Triple) :
>> >> OSTargetInfo<Target>(Triple) {
>> >> - this->UserLabelPrefix = "";
>> >> this->TLSSupported = false;
>> >>
>> >> switch (Triple.getArch()) {
>> >> @@ -536,7 +525,6 @@ protected:
>> >> }
>> >> public:
>> >> BitrigTargetInfo(const llvm::Triple &Triple) :
>> >> OSTargetInfo<Target>(Triple) {
>> >> - this->UserLabelPrefix = "";
>> >> this->MCountName = "__mcount";
>> >> }
>> >> };
>> >> @@ -554,9 +542,7 @@ protected:
>> >> Builder.defineMacro("__ELF__");
>> >> }
>> >> public:
>> >> - PSPTargetInfo(const llvm::Triple &Triple) :
>> >> OSTargetInfo<Target>(Triple) {
>> >> - this->UserLabelPrefix = "";
>> >> - }
>> >> + PSPTargetInfo(const llvm::Triple &Triple) :
>> >> OSTargetInfo<Target>(Triple) {}
>> >> };
>> >>
>> >> // PS3 PPU Target
>> >> @@ -576,7 +562,6 @@ protected:
>> >> }
>> >> public:
>> >> PS3PPUTargetInfo(const llvm::Triple &Triple) :
>> >> OSTargetInfo<Target>(Triple) {
>> >> - this->UserLabelPrefix = "";
>> >> this->LongWidth = this->LongAlign = 32;
>> >> this->PointerWidth = this->PointerAlign = 32;
>> >> this->IntMaxType = TargetInfo::SignedLongLong;
>> >> @@ -604,7 +589,6 @@ public:
>> >>
>> >> // On PS4, TLS variable cannot be aligned to more than 32 bytes
>> >> (256
>> >> bits).
>> >> this->MaxTLSAlign = 256;
>> >> - this->UserLabelPrefix = "";
>> >>
>> >> switch (Triple.getArch()) {
>> >> default:
>> >> @@ -724,7 +708,6 @@ protected:
>> >>
>> >> public:
>> >> NaClTargetInfo(const llvm::Triple &Triple) :
>> >> OSTargetInfo<Target>(Triple) {
>> >> - this->UserLabelPrefix = "";
>> >> this->LongAlign = 32;
>> >> this->LongWidth = 32;
>> >> this->PointerAlign = 32;
>> >> @@ -778,7 +761,6 @@ public:
>> >> explicit WebAssemblyOSTargetInfo(const llvm::Triple &Triple)
>> >> : OSTargetInfo<Target>(Triple) {
>> >> this->MCountName = "__mcount";
>> >> - this->UserLabelPrefix = "";
>> >> this->TheCXXABI.set(TargetCXXABI::WebAssembly);
>> >> }
>> >> };
>> >> @@ -816,6 +798,7 @@ public:
>> >> SimdDefaultAlign = 128;
>> >> LongDoubleWidth = LongDoubleAlign = 128;
>> >> LongDoubleFormat = &llvm::APFloat::PPCDoubleDouble;
>> >> + UserLabelPrefix = "_";
>> >> }
>> >>
>> >> /// \brief Flags for architecture specific defines.
>> >> @@ -1631,6 +1614,7 @@ public:
>> >> NoAsmVariants = true;
>> >> // Set the default GPU to sm20
>> >> GPU = GK_SM20;
>> >> + UserLabelPrefix = "_";
>> >> }
>> >> void getTargetDefines(const LangOptions &Opts,
>> >> MacroBuilder &Builder) const override {
>> >> @@ -3671,6 +3655,8 @@ public:
>> >> // FIXME: Check that we actually have cmpxchg8b before setting
>> >> // MaxAtomicInlineWidth. (cmpxchg8b is an i586 instruction.)
>> >> MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;
>> >> +
>> >> + UserLabelPrefix = "_";
>> >> }
>> >> BuiltinVaListKind getBuiltinVaListKind() const override {
>> >> return TargetInfo::CharPtrBuiltinVaList;
>> >> @@ -3882,7 +3868,6 @@ public:
>> >> IntPtrType = SignedLong;
>> >> PtrDiffType = SignedLong;
>> >> ProcessIDType = SignedLong;
>> >> - this->UserLabelPrefix = "";
>> >> this->TLSSupported = false;
>> >> }
>> >> void getTargetDefines(const LangOptions &Opts,
>> >> @@ -3929,8 +3914,6 @@ protected:
>> >>
>> >> public:
>> >> RTEMSTargetInfo(const llvm::Triple &Triple) :
>> >> OSTargetInfo<Target>(Triple) {
>> >> - this->UserLabelPrefix = "";
>> >> -
>> >> switch (Triple.getArch()) {
>> >> default:
>> >> case llvm::Triple::x86:
>> >> @@ -3957,7 +3940,6 @@ public:
>> >> SizeType = UnsignedLong;
>> >> IntPtrType = SignedLong;
>> >> PtrDiffType = SignedLong;
>> >> - this->UserLabelPrefix = "";
>> >> }
>> >> void getTargetDefines(const LangOptions &Opts,
>> >> MacroBuilder &Builder) const override {
>> >> @@ -4005,6 +3987,8 @@ public:
>> >> // x86-64 has atomics up to 16 bytes.
>> >> MaxAtomicPromoteWidth = 128;
>> >> MaxAtomicInlineWidth = 128;
>> >> +
>> >> + UserLabelPrefix = "_";
>> >> }
>> >> BuiltinVaListKind getBuiltinVaListKind() const override {
>> >> return TargetInfo::X86_64ABIBuiltinVaList;
>> >> @@ -4060,7 +4044,6 @@ public:
>> >> SizeType = UnsignedLongLong;
>> >> PtrDiffType = SignedLongLong;
>> >> IntPtrType = SignedLongLong;
>> >> - this->UserLabelPrefix = "";
>> >> }
>> >>
>> >> void getTargetDefines(const LangOptions &Opts,
>> >> @@ -4543,6 +4526,8 @@ public:
>> >> // that follows it, `bar', `bar' will be aligned as the type of
>> >> the
>> >> // zero length bitfield.
>> >> UseZeroLengthBitfieldAlignment = true;
>> >> +
>> >> + UserLabelPrefix = "_";
>> >> }
>> >>
>> >> StringRef getABI() const override { return ABI; }
>> >> @@ -5120,7 +5105,6 @@ public:
>> >> TLSSupported = false;
>> >> WCharType = UnsignedShort;
>> >> SizeType = UnsignedInt;
>> >> - UserLabelPrefix = "";
>> >> }
>> >> void getVisualStudioDefines(const LangOptions &Opts,
>> >> MacroBuilder &Builder) const {
>> >> @@ -5320,6 +5304,8 @@ public:
>> >>
>> >> // AArch64 targets default to using the ARM C++ ABI.
>> >> TheCXXABI.set(TargetCXXABI::GenericAArch64);
>> >> +
>> >> + UserLabelPrefix = "_";
>> >> }
>> >>
>> >> StringRef getABI() const override { return ABI; }
>> >> @@ -5844,7 +5830,9 @@ class SparcTargetInfo : public TargetInf
>> >> bool SoftFloat;
>> >> public:
>> >> SparcTargetInfo(const llvm::Triple &Triple)
>> >> - : TargetInfo(Triple), SoftFloat(false) {}
>> >> + : TargetInfo(Triple), SoftFloat(false) {
>> >> + UserLabelPrefix = "_";
>> >> + }
>> >>
>> >> bool handleTargetFeatures(std::vector<std::string> &Features,
>> >> DiagnosticsEngine &Diags) override {
>> >> @@ -6145,6 +6133,7 @@ public:
>> >> MinGlobalAlign = 16;
>> >> DataLayoutString =
>> >> "E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-a:8:16-n32:64";
>> >> MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;
>> >> + UserLabelPrefix = "_";
>> >> }
>> >> void getTargetDefines(const LangOptions &Opts,
>> >> MacroBuilder &Builder) const override {
>> >> @@ -6304,6 +6293,7 @@ public:
>> >> PtrDiffType = SignedInt;
>> >> SigAtomicType = SignedLong;
>> >> DataLayoutString = "e-m:e-p:16:16-i32:16:32-a:16-n8:16";
>> >> + UserLabelPrefix = "_";
>> >> }
>> >> void getTargetDefines(const LangOptions &Opts,
>> >> MacroBuilder &Builder) const override {
>> >> @@ -6400,6 +6390,7 @@ public:
>> >> "-f64:32-v64:32-v128:32-a:0:32-n32";
>> >> AddrSpaceMap = &TCEOpenCLAddrSpaceMap;
>> >> UseAddrSpaceMapMangling = true;
>> >> + UserLabelPrefix = "_";
>> >> }
>> >>
>> >> void getTargetDefines(const LangOptions &Opts,
>> >> @@ -6502,6 +6493,7 @@ public:
>> >> IsNan2008(false), IsSingleFloat(false), FloatABI(HardFloat),
>> >> DspRev(NoDSP), HasMSA(false), HasFP64(false), ABI(ABIStr) {
>> >> TheCXXABI.set(TargetCXXABI::GenericMIPS);
>> >> + UserLabelPrefix = "_";
>> >> }
>> >>
>> >> bool isNaN2008Default() const {
>> >> @@ -7078,7 +7070,6 @@ class PNaClTargetInfo : public TargetInf
>> >> public:
>> >> PNaClTargetInfo(const llvm::Triple &Triple) : TargetInfo(Triple) {
>> >> BigEndian = false;
>> >> - this->UserLabelPrefix = "";
>> >> this->LongAlign = 32;
>> >> this->LongWidth = 32;
>> >> this->PointerAlign = 32;
>> >>
>> >>
>> >> _______________________________________________
>> >> cfe-commits mailing list
>> >> cfe-commits at lists.llvm.org
>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >
>> >
More information about the cfe-commits
mailing list