Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Differential D27449 Diff 81398 include/llvm/MC/SectionKind.h

Changeset View

Standalone View

View Options

include/llvm/MC/SectionKind.h

Show All 22 Lines
class SectionKind { class SectionKind {
enum Kind { enum Kind {
/// Metadata - Debug info sections or other metadata. /// Metadata - Debug info sections or other metadata.
Metadata, Metadata,
/// Text - Text section, used for functions and other executable code. /// Text - Text section, used for functions and other executable code.
Text, Text,
/// ExecuteOnly, Text section that is not readable.
ExecuteOnly,
rengolinUnsubmitted
Not Done
ReplyInline Actions

No TABs, please. Use clang-format on your patch to make sure it adheres to the coding standard.

rengolin: No TABs, please. Use clang-format on your patch to make sure it adheres to the coding standard.
prakharAuthorUnsubmitted
Not Done
ReplyInline Actions

This isn't a tab character. Unless you meant that ExecuteOnly should be placed at the same indent level as Text?

prakhar: This isn't a tab character. Unless you meant that ExecuteOnly should be placed at the same…
rengolinUnsubmitted
Not Done
ReplyInline Actions

yup

rengolin: yup
/// ReadOnly - Data that is never written to at program runtime by the /// ReadOnly - Data that is never written to at program runtime by the
/// program or the dynamic linker. Things in the top-level readonly /// program or the dynamic linker. Things in the top-level readonly
/// SectionKind are not mergeable. /// SectionKind are not mergeable.
ReadOnly, ReadOnly,
/// MergableCString - Any null-terminated string which allows merging. /// MergableCString - Any null-terminated string which allows merging.
/// These values are known to end in a nul value of the specified size, /// These values are known to end in a nul value of the specified size,
/// not otherwise contain a nul value, and be mergable. This allows the /// not otherwise contain a nul value, and be mergable. This allows the
▲ Show 20 Lines • Show All 68 Lines • ▼ Show 20 Lines /// to during program runtime.
/// can write to them. If it chooses to, the dynamic linker can /// can write to them. If it chooses to, the dynamic linker can
/// mark the pages these globals end up on as read-only after it is /// mark the pages these globals end up on as read-only after it is
/// done with its relocation phase. /// done with its relocation phase.
ReadOnlyWithRel ReadOnlyWithRel
} K : 8; } K : 8;
public: public:
bool isMetadata() const { return K == Metadata; } bool isMetadata() const { return K == Metadata; }
bool isText() const { return K == Text; }
bool isText() const { return K == Text || K == ExecuteOnly; }
rengolinUnsubmitted
Not Done
ReplyInline Actions

To me it seems ExecuteOnly is orthogonal to isText. The problem is that we're using ReadOnly on the same solution space.

I think we should keep the accessors as simple as possible and use isText() || isExecuteOnly() in code.

rengolin: To me it seems `ExecuteOnly` is orthogonal to `isText`. The problem is that we're using…
prakharAuthorUnsubmitted
Not Done
ReplyInline Actions

As implemented here, an execute-only section is essentially a specialised text section for all purposes, with the added restriction that reads should not be made from it. I'm not sure the two are orthogonal.

prakhar: As implemented here, an execute-only section is essentially a specialised text section for all…
grosbachUnsubmitted
Not Done
ReplyInline Actions

Yeesh. This file is a bit of a mess. I'd much rather see ExecuteOnly be an attribute of Text rather than a completely distinct Kind. That's not the way the rest of the stuff is represented though, leading to the comment indentation mess Renato commented on.

Bottom line, this is fine as-is here, I'm just grumbling about the overall file. :)

grosbach: Yeesh. This file is a bit of a mess. I'd much rather see ExecuteOnly be an attribute of Text…
rengolinUnsubmitted
Not Done
ReplyInline Actions

Yeah, so, after I saw *all* the other options, I realised this split was the least of our worries. :)

My knowledge of this code is limited, so I'm not sure I could suggest a way to make this separation consistent everywhere else, and I'd certainly dislike proposing something ARM-specific here.

rengolin: Yeah, so, after I saw *all* the other options, I realised this split was the least of our…
bool isExecuteOnly() const { return K == ExecuteOnly; }
bool isReadOnly() const { bool isReadOnly() const {
return K == ReadOnly || isMergeableCString() || return K == ReadOnly || isMergeableCString() ||
isMergeableConst(); isMergeableConst();
} }
bool isMergeableCString() const { bool isMergeableCString() const {
return K == Mergeable1ByteCString || K == Mergeable2ByteCString || return K == Mergeable1ByteCString || K == Mergeable2ByteCString ||
▲ Show 20 Lines • Show All 43 Lines • ▼ Show 20 Lines static SectionKind get(Kind K) {
SectionKind Res; SectionKind Res;
Res.K = K; Res.K = K;
return Res; return Res;
} }
public: public:
static SectionKind getMetadata() { return get(Metadata); } static SectionKind getMetadata() { return get(Metadata); }
static SectionKind getText() { return get(Text); } static SectionKind getText() { return get(Text); }
static SectionKind getExecuteOnly() { return get(ExecuteOnly); }
static SectionKind getReadOnly() { return get(ReadOnly); } static SectionKind getReadOnly() { return get(ReadOnly); }
static SectionKind getMergeable1ByteCString() { static SectionKind getMergeable1ByteCString() {
return get(Mergeable1ByteCString); return get(Mergeable1ByteCString);
} }
static SectionKind getMergeable2ByteCString() { static SectionKind getMergeable2ByteCString() {
return get(Mergeable2ByteCString); return get(Mergeable2ByteCString);
} }
static SectionKind getMergeable4ByteCString() { static SectionKind getMergeable4ByteCString() {
Show All 19 Lines