This is an archive of the discontinued Mercurial Phabricator instance.

cdatapack: add some windows portability
ClosedPublic

Authored by ikostia on Jul 17 2017, 11:24 AM.

Download Raw Diff

Details

Reviewers

durham
dsp

Group Reviewers

Restricted Project

Commits

rFBHGXb8a935ce949b: cdatapack: add some windows portability

Summary

This is a first set of changes to help cdatapack compile on Windows. Second
set will include adding some way of using mman on Windows.

Test Plan

make local on Linux, rt
with some intermediary solution for mman this also builds on Windows 10, I was able to produce cdatapack_get.exe and cdatapack_dump.exe. Here's an example:

PS C:\Code\fb-hg-rpms\fb-hgext\cdatapack> .\cdatapack_get.exe 3ba0b10b8d251743a2692e042b114c1204b19d74 88dadb363234ec4fec3df85810810d6073288350

xplat/third-party/yarn/offline-mirror/smoothscroll-polyfill-0.3.5.tgz
Node                                      Delta Base                                Delta SHA1                                Delta Length
88dadb363234ec4fec3df85810810d6073288350  0000000000000000000000000000000000000000  466e6039b51cb525d70e1a5077ef81e064678eae  26057

Diff Detail

Repository

rFBHGX Facebook Mercurial Extensions

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ikostia created this revision.Jul 17 2017, 11:24 AM

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 17 2017, 11:24 AM

ikostia added a reviewer: dsp.Jul 17 2017, 11:31 AM

ikostia added inline comments.Jul 17 2017, 11:34 AM

clib/portability/portability.h
17	This is bad, but according to @durham ok for `cdatapack` purposes.

durham added inline comments.Jul 17 2017, 12:22 PM

clib/portability/inet.h
7	What does this line do? Does it need a comment?
clib/portability/portability.h
17	Hmm, I'm not sure this is safe. I think the code is defining these structs such that they match the on disk format exactly. We'd need to make sure visual studio is doing the same thing.

ikostia planned changes to this revision.Jul 17 2017, 12:58 PM

ikostia added inline comments.

clib/portability/inet.h
7	I think it hints Windows that this `.lib` file is needed at link time..? Honestly, I just copied it from https://msdn.microsoft.com/en-us/library/windows/desktop/ms737629(v=vs.85).aspx, but I can read up on it I guess.
clib/portability/portability.h
17	Let me look into https://msdn.microsoft.com/en-us/library/2e70t5y1.aspx, potentially we can use it to achieve the same thing.

fix structure packing portability

ikostia marked 3 inline comments as done.Jul 17 2017, 6:39 PM

some nits

Could you build and run the tests on linux to make sure the new packed declartion stuff passes there?

Edit: though perhaps you already did, based on the test plan

This revision is now accepted and ready to land.Jul 18 2017, 5:51 AM

Isn't that what I did in the test plan?)

Yea, my bad. Didn't read it thoroughly enough

Closed by commit rFBHGXb8a935ce949b: cdatapack: add some windows portability (authored by ikostia). · Explain WhyJul 18 2017, 6:25 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

		Path
M		cdatapack/cdatapack.c (18 lines)
A	M	clib/portability/inet.h (13 lines)
M		clib/portability/portability.h (7 lines)
A	M	clib/portability/unistd.h (16 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	203		Jul 17 2017, 11:24 AM	★	★
Diff 2	222	fix structure packing portability	Jul 17 2017, 6:37 PM	★	★
Diff 3	223	some nits	Jul 17 2017, 6:46 PM	★	★
Diff 4	242	rFBHGXb8a935ce949bb2c23006745b6a3f692e558713c8	Jul 18 2017, 6:21 AM	★	★

Diff 242

cdatapack/cdatapack.c

	#endif /* ndef _DEFAULT_SOURCE */			#endif /* ndef _DEFAULT_SOURCE */
	#ifndef _BSD_SOURCE			#ifndef _BSD_SOURCE
	#define _BSD_SOURCE			#define _BSD_SOURCE
	#endif /* ndef _BSD_SOURCE */			#endif /* ndef _BSD_SOURCE */

	#include <errno.h>			#include <errno.h>
	#include <fcntl.h>			#include <fcntl.h>
	#include <memory.h>			#include <memory.h>
	#include <unistd.h>
	#include <arpa/inet.h>
	#include <sys/mman.h>			#include <sys/mman.h>

	#if defined(__linux__)			#if defined(__linux__)
	#include <endian.h>			#include <endian.h>
	#define ntohll be64toh			#define ntohll be64toh

	// NOTE: we actually want MADV_FREE, because we only want to mark the page as			// NOTE: we actually want MADV_FREE, because we only want to mark the page as
	// eligible for immediate reuse while retaining the data. however, the exciting			// eligible for immediate reuse while retaining the data. however, the exciting
	// centos6 machines we have too old of a version of linux for MADV_FREE. LOL.			// centos6 machines we have too old of a version of linux for MADV_FREE. LOL.
	#define MADVISE_FREE_CODE MADV_DONTNEED			#define MADVISE_FREE_CODE MADV_DONTNEED
	#define PAGE_SIZE 4096			#define PAGE_SIZE 4096
	#endif /* #if defined(__linux__) */			#endif /* #if defined(__linux__) */

	#if defined(__APPLE__)			#if defined(__APPLE__)
	#define MADVISE_FREE_CODE MADV_FREE			#define MADVISE_FREE_CODE MADV_FREE
	#endif /* #if defined(__APPLE__) */			#endif /* #if defined(__APPLE__) */

	#include <lz4.h>			#include <lz4.h>
				#include "portability/inet.h"
				#include "portability/unistd.h"

	#include "cdatapack.h"			#include "cdatapack.h"
	#include "buffer.h"			#include "buffer.h"

	#define MAX_PAGED_IN_DATAPACK (1024 * 1024 * 1024)			#define MAX_PAGED_IN_DATAPACK (1024 * 1024 * 1024)

	/**			/**
	* This is an exact representation of an index entry on disk. Do not consume			* This is an exact representation of an index entry on disk. Do not consume
	* the fields directly, as they may need processing.			* the fields directly, as they may need processing.
	*
	* NOTE: this uses gcc's __attribute__((packed)) syntax to indicate a packed
	* data structure, which obviously has potential portability issues.
	*/			*/
	typedef struct _disk_index_entry_t {			PACKEDSTRUCT(typedef struct _disk_index_entry_t {
	uint8_t node[NODE_SZ];			uint8_t node[NODE_SZ];

	// offset of the next element in the delta chain in the index file			// offset of the next element in the delta chain in the index file
	index_offset_t deltabase_index_offset;			index_offset_t deltabase_index_offset;

	// offset and size of this current element in the delta chain in the data			// offset and size of this current element in the delta chain in the data
	// file.			// file.
	data_offset_t data_offset;			data_offset_t data_offset;
	data_offset_t data_sz;			data_offset_t data_sz;
	} __attribute__((packed)) disk_index_entry_t;			}) disk_index_entry_t;

	/**			/**
	* This represents offsets into the index indicating the range of a fanout			* This represents offsets into the index indicating the range of a fanout
	* bucket. This is calculated upon opening the file.			* bucket. This is calculated upon opening the file.
	*/			*/
	typedef struct _fanout_table_entry_t {			typedef struct _fanout_table_entry_t {
	index_offset_t start_index;			index_offset_t start_index;
	index_offset_t end_index;			index_offset_t end_index;
	pack_index_entry_t *pack_chain_links;			pack_index_entry_t *pack_chain_links;
	size_t links_idx;			size_t links_idx;
	size_t links_sz;			size_t links_sz;
	} pack_chain_t;			} pack_chain_t;

	/**			/**
	* This is an exact representation of an index file's header on disk. Do not			* This is an exact representation of an index file's header on disk. Do not
	* consume the fields directly, as they may need processing.			* consume the fields directly, as they may need processing.
	*
	* NOTE: this uses gcc's __attribute__((packed)) syntax to indicate a packed
	* data structure, which obviously has potential portability issues.
	*/			*/
	typedef struct _disk_index_header_t {			PACKEDSTRUCT(typedef struct _disk_index_header_t {
	#define VERSION 0			#define VERSION 0
	uint8_t version;			uint8_t version;

	#define LARGE_FANOUT 0x80			#define LARGE_FANOUT 0x80
	uint8_t config;			uint8_t config;
	} __attribute__((packed)) disk_index_header_t;			}) disk_index_header_t;

	static void unpack_disk_deltachunk(			static void unpack_disk_deltachunk(
	const disk_index_entry_t *disk_deltachunk,			const disk_index_entry_t *disk_deltachunk,
	pack_index_entry_t *packindex) {			pack_index_entry_t *packindex) {
	packindex->node = disk_deltachunk->node;			packindex->node = disk_deltachunk->node;
	packindex->data_offset = ntoh_data_offset(			packindex->data_offset = ntoh_data_offset(
	disk_deltachunk->data_offset);			disk_deltachunk->data_offset);
	packindex->data_sz = ntoh_data_offset(			packindex->data_sz = ntoh_data_offset(

clib/portability/inet.h

This file was added.

				#ifndef PORTABILITY_INET_H
				#define PORTABILITY_INET_H

				#if defined(_MSC_VER)
				#include <winsock2.h>
				#pragma comment(lib, "Ws2_32.lib")
				/* See https://fburl.com/7hd350j8 for more details about Ws2_32.lib */
				durhamUnsubmitted Not Done What does this line do? Does it need a comment? durham: What does this line do? Does it need a comment?
				ikostiaAuthorUnsubmitted Not Done I think it hints Windows that this `.lib` file is needed at link time..? Honestly, I just copied it from https://msdn.microsoft.com/en-us/library/windows/desktop/ms737629(v=vs.85).aspx, but I can read up on it I guess. ikostia: I think it hints Windows that this `.lib` file is needed at link time..? Honestly, I just…
				#else
				#include <arpa/inet.h>
				#endif

				#endif /* PORTABILITY_INET_H */

clib/portability/portability.h

	#ifndef PORTABILITY_PORTABILITY_H			#ifndef PORTABILITY_PORTABILITY_H
	#define PORTABILITY_PORTABILITY_H			#define PORTABILITY_PORTABILITY_H

	#if defined(_MSC_VER)			#if defined(_MSC_VER)
	/* MSVC2015 supports compound literals in C mode (/TC)			/* MSVC2015 supports compound literals in C mode (/TC)
	but does not support them in C++ mode (/TP) */			but does not support them in C++ mode (/TP) */
	#if defined(__cplusplus)			#if defined(__cplusplus)
	#define COMPOUND_LITERAL(typename_) typename_			#define COMPOUND_LITERAL(typename_) typename_
	#else /* #if defined(__cplusplus) */			#else /* #if defined(__cplusplus) */
	#define COMPOUND_LITERAL(typename_) (typename_)			#define COMPOUND_LITERAL(typename_) (typename_)
	#endif /* #if defined(__cplusplus) */			#endif /* #if defined(__cplusplus) */
	#else /* #if defined(_MSC_VER) */			#else /* #if defined(_MSC_VER) */
	#define COMPOUND_LITERAL(typename_) (typename_)			#define COMPOUND_LITERAL(typename_) (typename_)
	#endif /* #if defined(_MSC_VER) */			#endif /* #if defined(_MSC_VER) */

				#if defined(_MSC_VER)
				#define PACKEDSTRUCT(__Declaration__) __pragma(pack(push, 1)) \
				ikostiaAuthorUnsubmitted Done This is bad, but according to @durham ok for `cdatapack` purposes. ikostia: This is bad, but according to @durham ok for `cdatapack` purposes.
				durhamUnsubmitted Done Hmm, I'm not sure this is safe. I think the code is defining these structs such that they match the on disk format exactly. We'd need to make sure visual studio is doing the same thing. durham: Hmm, I'm not sure this is safe. I think the code is defining these structs such that they…
				ikostiaAuthorUnsubmitted Done Let me look into https://msdn.microsoft.com/en-us/library/2e70t5y1.aspx, potentially we can use it to achieve the same thing. ikostia: Let me look into https://msdn.microsoft.com/en-us/library/2e70t5y1.aspx, potentially we can use…
				__Declaration__ __pragma(pack(pop))
				#else
				#define PACKEDSTRUCT(__Declaration__) __Declaration__ __attribute__((packed))
				#endif

	#endif /* #ifndef PORTABILITY_PORTABILITY_H */			#endif /* #ifndef PORTABILITY_PORTABILITY_H */

clib/portability/unistd.h

This file was added.

				#ifndef PORTABILITY_UNISTD_H
				#define PORTABILITY_UNISTD_H

				#if defined(_MSC_VER)
				#include <io.h>
				/* MSVC's io.h header shows deprecation
				warnings on these without underscore */
				#define lseek _lseek
				#define open _open
				#define close _close
				#else
				#include <unistd.h>
				#endif

				#endif /* PORTABILITY_UNISTD_H */