[LLVM] [Clang] Backport "Support for Gentoo *t64 triples (64-bit time_t ABIs)" by mgorny · Pull Request #112364 · llvm/llvm-project (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

@mgorny

This is a backport of 387b37a for 19.x, adjusted to add new Triple::EnvironmentType members at the end to avoid breaking backwards ABI compatibility.

Gentoo is planning to introduce a *t64 suffix for triples that will be used by 32-bit platforms that use 64-bit time_t. Add support for parsing and accepting these triples, and while at it make clang automatically enable the necessary glibc feature macros when this suffix is used.

@llvmbot

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Michał Górny (mgorny)

Changes

This is a backport of 387b37a for 19.x, adjusted to add new Triple::EnvironmentType members at the end to avoid breaking backwards ABI compatibility.

Gentoo is planning to introduce a *t64 suffix for triples that will be used by 32-bit platforms that use 64-bit time_t. Add support for parsing and accepting these triples, and while at it make clang automatically enable the necessary glibc feature macros when this suffix is used.

Full diff: https://github.com/llvm/llvm-project/pull/112364.diff

16 Files Affected:

diff --git a/clang/lib/Basic/Targets/ARM.cpp b/clang/lib/Basic/Targets/ARM.cpp index 7423626d7c3cbf..e55feedbd5c6f9 100644 --- a/clang/lib/Basic/Targets/ARM.cpp +++ b/clang/lib/Basic/Targets/ARM.cpp @@ -311,7 +311,9 @@ ARMTargetInfo::ARMTargetInfo(const llvm::Triple &Triple, switch (Triple.getEnvironment()) { case llvm::Triple::Android: case llvm::Triple::GNUEABI:

}

public: diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp index cf5e29e5a3db8d..8d9beab9fa7f88 100644 --- a/clang/lib/CodeGen/CodeGenModule.cpp +++ b/clang/lib/CodeGen/CodeGenModule.cpp @@ -177,10 +177,7 @@ createTargetCodeGenInfo(CodeGenModule &CGM) { else if (ABIStr == "aapcs16") Kind = ARMABIKind::AAPCS16_VFP; else if (CodeGenOpts.FloatABI == "hard" ||

diff --git a/clang/lib/CodeGen/Targets/ARM.cpp b/clang/lib/CodeGen/Targets/ARM.cpp index d032b88d7683cd..457d761039a08d 100644 --- a/clang/lib/CodeGen/Targets/ARM.cpp +++ b/clang/lib/CodeGen/Targets/ARM.cpp @@ -35,7 +35,9 @@ class ARMABIInfo : public ABIInfo { case llvm::Triple::EABI: case llvm::Triple::EABIHF: case llvm::Triple::GNUEABI:

@@ -665,11 +666,13 @@ static llvm::Triple computeTargetTriple(const Driver &D, } else if (ABIName == "n32") { Target = Target.get64BitArchVariant(); if (Target.getEnvironment() == llvm::Triple::GNU ||

diff --git a/clang/lib/Driver/ToolChains/Arch/ARM.cpp b/clang/lib/Driver/ToolChains/Arch/ARM.cpp index a6041b809b80b6..0489911ecd9dee 100644 --- a/clang/lib/Driver/ToolChains/Arch/ARM.cpp +++ b/clang/lib/Driver/ToolChains/Arch/ARM.cpp @@ -327,6 +327,11 @@ void arm::setFloatABIInTriple(const Driver &D, const ArgList &Args, Triple.setEnvironment(isHardFloat ? llvm::Triple::GNUEABIHF : llvm::Triple::GNUEABI); break;

case llvm::Triple::EABI: case llvm::Triple::EABIHF: Triple.setEnvironment(isHardFloat ? llvm::Triple::EABIHF @@ -414,10 +419,12 @@ arm::FloatABI arm::getDefaultFloatABI(const llvm::Triple &Triple) { return FloatABI::Soft; switch (Triple.getEnvironment()) { case llvm::Triple::GNUEABIHF:

@@ -2705,6 +2706,7 @@ void Generic_GCC::GCCInstallationDetector::AddDefaultGCCPrefixes( case llvm::Triple::thumbeb: LibDirs.append(begin(ARMebLibDirs), end(ARMebLibDirs)); if (TargetTriple.getEnvironment() == llvm::Triple::GNUEABIHF ||

diff --git a/clang/lib/Driver/ToolChains/Linux.cpp b/clang/lib/Driver/ToolChains/Linux.cpp index 2265138edbffbe..35bf3906960509 100644 --- a/clang/lib/Driver/ToolChains/Linux.cpp +++ b/clang/lib/Driver/ToolChains/Linux.cpp @@ -508,6 +508,7 @@ std::string Linux::getDynamicLinker(const ArgList &Args) const { case llvm::Triple::thumbeb: { const bool HF = Triple.getEnvironment() == llvm::Triple::GNUEABIHF ||

diff --git a/clang/test/Preprocessor/time64.c b/clang/test/Preprocessor/time64.c new file mode 100644 index 00000000000000..e9e26e08ecd870 --- /dev/null +++ b/clang/test/Preprocessor/time64.c @@ -0,0 +1,12 @@ +// RUN: %clang_cc1 -E -dM -triple=i686-pc-linux-gnu /dev/null | FileCheck -match-full-lines -check-prefix TIME32 %s +// RUN: %clang_cc1 -E -dM -triple=i686-pc-linux-gnut64 /dev/null | FileCheck -match-full-lines -check-prefix TIME64 %s +// RUN: %clang_cc1 -E -dM -triple=armv5tel-softfloat-linux-gnueabi /dev/null | FileCheck -match-full-lines -check-prefix TIME32 %s +// RUN: %clang_cc1 -E -dM -triple=armv5tel-softfloat-linux-gnueabit64 /dev/null | FileCheck -match-full-lines -check-prefix TIME64 %s +// RUN: %clang_cc1 -E -dM -triple=armv7a-unknown-linux-gnueabihf /dev/null | FileCheck -match-full-lines -check-prefix TIME32 %s +// RUN: %clang_cc1 -E -dM -triple=armv7a-unknown-linux-gnueabihft64 /dev/null | FileCheck -match-full-lines -check-prefix TIME64 %s +// +// TIME32-NOT:#define _FILE_OFFSET_BITS 64 +// TIME32-NOT:#define _TIME_BITS 64 +// +// TIME64:#define _FILE_OFFSET_BITS 64 +// TIME64:#define _TIME_BITS 64 diff --git a/llvm/include/llvm/TargetParser/Triple.h b/llvm/include/llvm/TargetParser/Triple.h index ebd92f264d9045..d2126a03db906a 100644 --- a/llvm/include/llvm/TargetParser/Triple.h +++ b/llvm/include/llvm/TargetParser/Triple.h @@ -294,7 +294,11 @@ class Triple {

 PAuthTest,

}; enum ObjectFormatType { UnknownObjectFormat, @@ -605,11 +609,12 @@ class Triple {

bool isGNUEnvironment() const { EnvironmentType Env = getEnvironment();

@@ -866,9 +871,11 @@ class Triple { return (isARM() || isThumb()) && (getEnvironment() == Triple::EABI || getEnvironment() == Triple::GNUEABI ||

@@ -1046,6 +1053,22 @@ class Triple { return getArch() == Triple::bpfel || getArch() == Triple::bpfeb; }

diff --git a/llvm/lib/Target/ARM/ARMSubtarget.h b/llvm/lib/Target/ARM/ARMSubtarget.h index 00239ff94b7ba5..6d301efd56180d 100644 --- a/llvm/lib/Target/ARM/ARMSubtarget.h +++ b/llvm/lib/Target/ARM/ARMSubtarget.h @@ -325,7 +325,9 @@ class ARMSubtarget : public ARMGenSubtargetInfo { } bool isTargetGNUAEABI() const { return (TargetTriple.getEnvironment() == Triple::GNUEABI ||

diff --git a/llvm/lib/Target/ARM/ARMTargetMachine.h b/llvm/lib/Target/ARM/ARMTargetMachine.h index 69d8fa8ada6498..75ee50f0e93c88 100644 --- a/llvm/lib/Target/ARM/ARMTargetMachine.h +++ b/llvm/lib/Target/ARM/ARMTargetMachine.h @@ -64,6 +64,7 @@ class ARMBaseTargetMachine : public LLVMTargetMachine {

bool isTargetHardFloat() const { return TargetTriple.getEnvironment() == Triple::GNUEABIHF ||

diff --git a/llvm/lib/TargetParser/ARMTargetParser.cpp b/llvm/lib/TargetParser/ARMTargetParser.cpp index 9d9917d86a368b..5e9dd94b84b28f 100644 --- a/llvm/lib/TargetParser/ARMTargetParser.cpp +++ b/llvm/lib/TargetParser/ARMTargetParser.cpp @@ -554,7 +554,9 @@ StringRef ARM::computeDefaultTargetABI(const Triple &TT, StringRef CPU) { switch (TT.getEnvironment()) { case Triple::Android: case Triple::GNUEABI:

@@ -701,6 +706,7 @@ static Triple::EnvironmentType parseEnvironment(StringRef EnvironmentName) { .StartsWith("gnux32", Triple::GNUX32) .StartsWith("gnu_ilp32", Triple::GNUILP32) .StartsWith("code16", Triple::CODE16)

diff --git a/llvm/unittests/TargetParser/TripleTest.cpp b/llvm/unittests/TargetParser/TripleTest.cpp index 0aecfc64da2080..ae5df4621df942 100644 --- a/llvm/unittests/TargetParser/TripleTest.cpp +++ b/llvm/unittests/TargetParser/TripleTest.cpp @@ -93,6 +93,13 @@ TEST(TripleTest, ParsedIDs) { EXPECT_EQ(Triple::Hurd, T.getOS()); EXPECT_EQ(Triple::GNU, T.getEnvironment());

@@ -1175,6 +1210,29 @@ TEST(TripleTest, ParsedIDs) { EXPECT_EQ(Triple::Linux, T.getOS()); EXPECT_EQ(Triple::PAuthTest, T.getEnvironment());

@llvmbot

@llvm/pr-subscribers-backend-arm

Author: Michał Górny (mgorny)

Changes

This is a backport of 387b37a for 19.x, adjusted to add new Triple::EnvironmentType members at the end to avoid breaking backwards ABI compatibility.

Gentoo is planning to introduce a *t64 suffix for triples that will be used by 32-bit platforms that use 64-bit time_t. Add support for parsing and accepting these triples, and while at it make clang automatically enable the necessary glibc feature macros when this suffix is used.

Full diff: https://github.com/llvm/llvm-project/pull/112364.diff

16 Files Affected:

diff --git a/clang/lib/Basic/Targets/ARM.cpp b/clang/lib/Basic/Targets/ARM.cpp index 7423626d7c3cbf..e55feedbd5c6f9 100644 --- a/clang/lib/Basic/Targets/ARM.cpp +++ b/clang/lib/Basic/Targets/ARM.cpp @@ -311,7 +311,9 @@ ARMTargetInfo::ARMTargetInfo(const llvm::Triple &Triple, switch (Triple.getEnvironment()) { case llvm::Triple::Android: case llvm::Triple::GNUEABI:

}

public: diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp index cf5e29e5a3db8d..8d9beab9fa7f88 100644 --- a/clang/lib/CodeGen/CodeGenModule.cpp +++ b/clang/lib/CodeGen/CodeGenModule.cpp @@ -177,10 +177,7 @@ createTargetCodeGenInfo(CodeGenModule &CGM) { else if (ABIStr == "aapcs16") Kind = ARMABIKind::AAPCS16_VFP; else if (CodeGenOpts.FloatABI == "hard" ||

diff --git a/clang/lib/CodeGen/Targets/ARM.cpp b/clang/lib/CodeGen/Targets/ARM.cpp index d032b88d7683cd..457d761039a08d 100644 --- a/clang/lib/CodeGen/Targets/ARM.cpp +++ b/clang/lib/CodeGen/Targets/ARM.cpp @@ -35,7 +35,9 @@ class ARMABIInfo : public ABIInfo { case llvm::Triple::EABI: case llvm::Triple::EABIHF: case llvm::Triple::GNUEABI:

@@ -665,11 +666,13 @@ static llvm::Triple computeTargetTriple(const Driver &D, } else if (ABIName == "n32") { Target = Target.get64BitArchVariant(); if (Target.getEnvironment() == llvm::Triple::GNU ||

diff --git a/clang/lib/Driver/ToolChains/Arch/ARM.cpp b/clang/lib/Driver/ToolChains/Arch/ARM.cpp index a6041b809b80b6..0489911ecd9dee 100644 --- a/clang/lib/Driver/ToolChains/Arch/ARM.cpp +++ b/clang/lib/Driver/ToolChains/Arch/ARM.cpp @@ -327,6 +327,11 @@ void arm::setFloatABIInTriple(const Driver &D, const ArgList &Args, Triple.setEnvironment(isHardFloat ? llvm::Triple::GNUEABIHF : llvm::Triple::GNUEABI); break;

case llvm::Triple::EABI: case llvm::Triple::EABIHF: Triple.setEnvironment(isHardFloat ? llvm::Triple::EABIHF @@ -414,10 +419,12 @@ arm::FloatABI arm::getDefaultFloatABI(const llvm::Triple &Triple) { return FloatABI::Soft; switch (Triple.getEnvironment()) { case llvm::Triple::GNUEABIHF:

@@ -2705,6 +2706,7 @@ void Generic_GCC::GCCInstallationDetector::AddDefaultGCCPrefixes( case llvm::Triple::thumbeb: LibDirs.append(begin(ARMebLibDirs), end(ARMebLibDirs)); if (TargetTriple.getEnvironment() == llvm::Triple::GNUEABIHF ||

diff --git a/clang/lib/Driver/ToolChains/Linux.cpp b/clang/lib/Driver/ToolChains/Linux.cpp index 2265138edbffbe..35bf3906960509 100644 --- a/clang/lib/Driver/ToolChains/Linux.cpp +++ b/clang/lib/Driver/ToolChains/Linux.cpp @@ -508,6 +508,7 @@ std::string Linux::getDynamicLinker(const ArgList &Args) const { case llvm::Triple::thumbeb: { const bool HF = Triple.getEnvironment() == llvm::Triple::GNUEABIHF ||

diff --git a/clang/test/Preprocessor/time64.c b/clang/test/Preprocessor/time64.c new file mode 100644 index 00000000000000..e9e26e08ecd870 --- /dev/null +++ b/clang/test/Preprocessor/time64.c @@ -0,0 +1,12 @@ +// RUN: %clang_cc1 -E -dM -triple=i686-pc-linux-gnu /dev/null | FileCheck -match-full-lines -check-prefix TIME32 %s +// RUN: %clang_cc1 -E -dM -triple=i686-pc-linux-gnut64 /dev/null | FileCheck -match-full-lines -check-prefix TIME64 %s +// RUN: %clang_cc1 -E -dM -triple=armv5tel-softfloat-linux-gnueabi /dev/null | FileCheck -match-full-lines -check-prefix TIME32 %s +// RUN: %clang_cc1 -E -dM -triple=armv5tel-softfloat-linux-gnueabit64 /dev/null | FileCheck -match-full-lines -check-prefix TIME64 %s +// RUN: %clang_cc1 -E -dM -triple=armv7a-unknown-linux-gnueabihf /dev/null | FileCheck -match-full-lines -check-prefix TIME32 %s +// RUN: %clang_cc1 -E -dM -triple=armv7a-unknown-linux-gnueabihft64 /dev/null | FileCheck -match-full-lines -check-prefix TIME64 %s +// +// TIME32-NOT:#define _FILE_OFFSET_BITS 64 +// TIME32-NOT:#define _TIME_BITS 64 +// +// TIME64:#define _FILE_OFFSET_BITS 64 +// TIME64:#define _TIME_BITS 64 diff --git a/llvm/include/llvm/TargetParser/Triple.h b/llvm/include/llvm/TargetParser/Triple.h index ebd92f264d9045..d2126a03db906a 100644 --- a/llvm/include/llvm/TargetParser/Triple.h +++ b/llvm/include/llvm/TargetParser/Triple.h @@ -294,7 +294,11 @@ class Triple {

 PAuthTest,

}; enum ObjectFormatType { UnknownObjectFormat, @@ -605,11 +609,12 @@ class Triple {

bool isGNUEnvironment() const { EnvironmentType Env = getEnvironment();

@@ -866,9 +871,11 @@ class Triple { return (isARM() || isThumb()) && (getEnvironment() == Triple::EABI || getEnvironment() == Triple::GNUEABI ||

@@ -1046,6 +1053,22 @@ class Triple { return getArch() == Triple::bpfel || getArch() == Triple::bpfeb; }

diff --git a/llvm/lib/Target/ARM/ARMSubtarget.h b/llvm/lib/Target/ARM/ARMSubtarget.h index 00239ff94b7ba5..6d301efd56180d 100644 --- a/llvm/lib/Target/ARM/ARMSubtarget.h +++ b/llvm/lib/Target/ARM/ARMSubtarget.h @@ -325,7 +325,9 @@ class ARMSubtarget : public ARMGenSubtargetInfo { } bool isTargetGNUAEABI() const { return (TargetTriple.getEnvironment() == Triple::GNUEABI ||

diff --git a/llvm/lib/Target/ARM/ARMTargetMachine.h b/llvm/lib/Target/ARM/ARMTargetMachine.h index 69d8fa8ada6498..75ee50f0e93c88 100644 --- a/llvm/lib/Target/ARM/ARMTargetMachine.h +++ b/llvm/lib/Target/ARM/ARMTargetMachine.h @@ -64,6 +64,7 @@ class ARMBaseTargetMachine : public LLVMTargetMachine {

bool isTargetHardFloat() const { return TargetTriple.getEnvironment() == Triple::GNUEABIHF ||

diff --git a/llvm/lib/TargetParser/ARMTargetParser.cpp b/llvm/lib/TargetParser/ARMTargetParser.cpp index 9d9917d86a368b..5e9dd94b84b28f 100644 --- a/llvm/lib/TargetParser/ARMTargetParser.cpp +++ b/llvm/lib/TargetParser/ARMTargetParser.cpp @@ -554,7 +554,9 @@ StringRef ARM::computeDefaultTargetABI(const Triple &TT, StringRef CPU) { switch (TT.getEnvironment()) { case Triple::Android: case Triple::GNUEABI:

@@ -701,6 +706,7 @@ static Triple::EnvironmentType parseEnvironment(StringRef EnvironmentName) { .StartsWith("gnux32", Triple::GNUX32) .StartsWith("gnu_ilp32", Triple::GNUILP32) .StartsWith("code16", Triple::CODE16)

diff --git a/llvm/unittests/TargetParser/TripleTest.cpp b/llvm/unittests/TargetParser/TripleTest.cpp index 0aecfc64da2080..ae5df4621df942 100644 --- a/llvm/unittests/TargetParser/TripleTest.cpp +++ b/llvm/unittests/TargetParser/TripleTest.cpp @@ -93,6 +93,13 @@ TEST(TripleTest, ParsedIDs) { EXPECT_EQ(Triple::Hurd, T.getOS()); EXPECT_EQ(Triple::GNU, T.getEnvironment());

@@ -1175,6 +1210,29 @@ TEST(TripleTest, ParsedIDs) { EXPECT_EQ(Triple::Linux, T.getOS()); EXPECT_EQ(Triple::PAuthTest, T.getEnvironment());

MaskRay

thesamesam

@tru

Correct me if I am wrong, but this seems like an added feature and not a regression or critical fix? it's a pretty big patch and while it's most likely is "safe" it would still fall outside our current definition of backports?

@mgorny

Correct me if I am wrong, but this seems like an added feature and not a regression or critical fix? it's a pretty big patch and while it's most likely is "safe" it would still fall outside our current definition of backports?

Without this change, you can't use clang at all, because it rejects the host triplet.

@mgorny

Just to clarify my concise reply (I was in a hurry): right now (FWICS since clang 17), clang rejects the host triplet we'd like to use as invalid, i.e.:

clang: error: version 't32' in target triple 'i686-pc-linux-gnut32' is invalid
clang: error: no input files

So at least some patch is needed to make it work at all. And since we're changing it anyway, I don't think there's a major risk in backporting the whole thing — and it would at least have the advantage of preserving consistent behavior across clang versions (i.e. not having clang 20 that applies time64 flags, and clang 19 that doesn't reject the triplet but doesn't apply the flags).

@mgorny @tru

…me_t ABIs)"

This is a backport of 387b37a for 19.x, adjusted to add new Triple::EnvironmentType members at the end to avoid breaking backwards ABI compatibility.

Gentoo is planning to introduce a *t64 suffix for triples that will be used by 32-bit platforms that use 64-bit time_t. Add support for parsing and accepting these triples, and while at it make clang automatically enable the necessary glibc feature macros when this suffix is used.

@github-actions

@mgorny (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

@mgorny

Added support for Gentoo Linux triplets suffixed -gnut64, -gnueabit64 and -gnueabihft64. On these targets, clang automatically defines -D_TIME_BITS=64 to use 64-bit time_t type from glibc.

alexrp

Comment on lines +297 to +301

GNUT64,
GNUEABIT64,
GNUEABIHFT64,
LastEnvironmentType = GNUEABIHFT64

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what the LLVM project's policy is on API/ABI stability of marker enum values like this one, but this broke Zig:

/home/andy/src/zig/src/zig_llvm.cpp:753:68: error: static assertion failed
  753 | static_assert((Triple::EnvironmentType)ZigLLVM_LastEnvironmentType == Triple::LastEnvironmentType, "");
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/andy/src/zig/src/zig_llvm.cpp:753:68: note: the comparison reduces to ‘(42 == 45)’

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. That looks a bit odd. The Abi checker didn't catch this. Wonder why - @tstellar

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah it was just adding to the end of the enum. That should be fine. It was hard to see the diff on mobile. I was worried it inserted something in the middle of the enum.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But in this case, the new members were, in effect, not appended, since LastEnvironmentType had its value change. Is the ABI checker deliberately ignoring marker enum members such as this one?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch doesn't break ABI/API compatibility. The Zig check is overly restrictive and unnecessary. Zig should be fixed instead.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check is intended so that we can find out when LLVM adds additional ABI tags to this enum. For example, it alerted us to this exact change. If the check was deleted as @MaskRay suggests, then we would have missed the fact that we need to add code elsewhere to handle these additional tags.

LLVM should not be modifying the triple in a patch release. This is not a bug fix, it is an enhancement. Just look at the commit message, it begins with the words "Support for ..."

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that used to be the case anyway. @alexrp pointed out in ziglang/zig#21862 that since we now generate LLVM bitcode rather than using the LLVM IRBuilder API, we can delete these strict checks.

So, this backport is not relevant to the zig project after all (even though it's still true that it's not a bug fix and pedantically speaking shouldn't have been backported).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch doesn't break ABI/API compatibility. The Zig check is overly restrictive and unnecessary. Zig should be fixed instead.

It does technically break ABI compatibility, because it changes the value of an enum. This is something we usually try to avoid.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewers

@alexrp alexrp alexrp left review comments

@andrewrk andrewrk andrewrk left review comments

@tstellar tstellar tstellar left review comments

@MaskRay MaskRay MaskRay approved these changes

@thesamesam thesamesam thesamesam approved these changes

@tru tru Awaiting requested review from tru

Labels