Skip to content

Add support for reading YAML legacy type info dump #19707

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Oct 5, 2018
Merged

Add support for reading YAML legacy type info dump #19707

merged 3 commits into from
Oct 5, 2018

Conversation

slavapestov
Copy link
Contributor

Progress on rdar://problem/17528739.

@slavapestov slavapestov changed the title Add support for reading YAML legacy type info dump Add support for reading YAML legacy type info dump [WIP] Oct 4, 2018
@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

@slavapestov slavapestov force-pushed the read-legacy-layout branch 2 times, most recently from b7499b9 to 384d91a Compare October 5, 2018 02:44
@slavapestov slavapestov changed the title Add support for reading YAML legacy type info dump [WIP] Add support for reading YAML legacy type info dump Oct 5, 2018
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

jrose-apple
jrose-apple reviewed Oct 5, 2018
View reviewed changes
Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

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

Looks pretty much like what we talked about – my comments are all little detail things. I think you should have a more IRGen-ish person look it over too, though, like Arnold or John.

include/swift/Option/FrontendOptions.td
@@ -497,6 +497,9 @@ def enable_class_resilience : Flag<["-"], "enable-class-resilience">,
def enable_resilience_bypass : Flag<["-"], "enable-resilience-bypass">,
HelpText<"Completely bypass resilience when accessing types in resilient frameworks">;

def read_type_info_path_EQ : Joined<["-"], "read-type-info-path=">,
HelpText<"Read legacy type layout from the given path">;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the majority of options use -foo blah rather than -foo=blah.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was just so that when testing I can write -Xfrontend -read-type-info-path=foo instead of -Xfrontend -read-type-info-path -Xfrontend foo. But I think this flag is only temporary, since we'll want to use some other mechanism to find these file(s) eventually, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, probably we'll just look in the resource directory. The -Xfrontend motivation is reasonable but I figure you're going to be using the flag mostly in tests, right?

lib/Frontend/CompilerInvocation.cpp Outdated
@@ -1023,6 +1023,11 @@ static bool ParseIRGenArgs(IRGenOptions &Opts, ArgList &Args,
Opts.EnableResilienceBypass = true;
}

if (const Arg *A = Args.getLastArg(OPT_read_type_info_path_EQ)) {
StringRef path(A->getValue());
Opts.ReadTypeInfoPath = path;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to do this in two steps.

lib/IRGen/GenType.cpp Outdated
}

Optional<YAMLTypeInfoNode>
TypeConverter::getLegacyTypeInfo(NominalTypeDecl *decl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: const

lib/IRGen/GenType.cpp Outdated
bool error = readLegacyTypeInfo(path);
if (error) {
llvm::errs() << "Cannot read '" << path << "'\n";
abort();
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use llvm::report_fatal_error for a slightly better aborting experience. Long-term, we may need to emit a proper diagnostic.

lib/IRGen/GenType.cpp Outdated
CompletelyFragile = true;
LoweringMode = Mode::CompletelyFragile;

auto path = IGM.IRGen.Opts.ReadTypeInfoPath;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: It'd be good to explicitly make this a StringRef, in case we need to change the option to a std::string.

lib/IRGen/GenType.cpp
Alignment(node.Alignment),
IsPOD, /* irrelevant */
IsBitwiseTakable, /* irrelevant */
IsFixedSize /* irrelevant */),
Copy link
Contributor

Choose a reason for hiding this comment

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

Since they're irrelevant, it's probably best to provide the safest options, i.e. non-POD, non-bitwise-takable…but yes fixed-size.

lib/IRGen/GenType.cpp Outdated
NumExtraInhabitants(node.NumExtraInhabitants) {}

// TODO: Override more methods to prevent you from doing anything with this
// TypeInfo?
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a good idea. Are there more than what you've already provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote this TODO before I added the ones I put in, so I'll remove it.

lib/IRGen/GenType.h Outdated
@@ -60,9 +62,29 @@ namespace irgen {
/// The helper class for generating types.
class TypeConverter {
public:
static unsigned const NumLoweringModes = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: can you put this after the Mode enum, to make it more likely we'll remember to update it?

@slavapestov
Copy link
Contributor Author

@rjmccall Want to take a quick look too?

@slavapestov
IRGen: Add a new 'Legacy' lowering mode that loads type info from a Y…
87ec607 
…AML file

The YAML format is the same one produced by the -dump-type-info
frontend mode.

For now this is only enabled if the -read-type-info-path frontend
flag is specified.

Progress on <rdar://problem/17528739>.
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test macOS

@slavapestov slavapestov merged commit 30afc54 into swiftlang:master Oct 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrose-apple jrose-apple jrose-apple left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@slavapestov @jrose-apple