min}.
ClosedPublic
Actions

Authored by kib on Aug 24 2018, 3:32 PM.

Details

Reviewers

alc
manu
markj
royger

Commits

rS338370: Remove {max/min}_offset() macros, use vm_map_{max/min}() inlines.

Summary

Exposing max_offset and min_offset defines in public headers is causing
clashes with variable names, for example when building QEMU:

In file included from /root/src/xen/tools/qemu-xen-dir/hw/usb/xen-usb.c:30:
In file included from /root/src/xen/tools/qemu-xen-dir/include/hw/xen/xen_backend.h:4:
In file included from /root/src/xen/tools/qemu-xen-dir/include/hw/xen/xen_common.h:19:
In file included from /root/src/xen/tools/qemu-xen-dir/include/hw/pci/pci.h:6:
In file included from /root/src/xen/tools/qemu-xen-dir/include/sysemu/dma.h:16:
In file included from /root/src/xen/tools/qemu-xen-dir/include/block/block.h:10:
/root/src/xen/tools/qemu-xen-dir/include/block/dirty-bitmap.h:86:68: error: expected ')'
bool bdrv_dirty_iter_next_area(BdrvDirtyBitmapIter *iter, uint64_t max_offset,

/usr/include/vm/vm_map.h:188:26: note: expanded from macro 'max_offset'
#define max_offset header.start /* (c) */

/root/src/xen/tools/qemu-xen-dir/include/block/dirty-bitmap.h:86:31: note: to match this '('
bool bdrv_dirty_iter_next_area(BdrvDirtyBitmapIter *iter, uint64_t max_offset,

Test Plan

Only tested with an amd64 buildkernel so far, will run a tinderbox before committing to make sure everything is fine.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 19135
Build 18760: arc lint + arc unit

Event Timeline

royger created this revision.Aug 24 2018, 3:32 PM

Herald added a reviewer: manu. · View Herald TranscriptAug 24 2018, 3:32 PM

Herald added subscribers: emaste, andrew, delphij. · View Herald Transcript

Harbormaster completed remote builds in B19135: Diff 47237.Aug 24 2018, 3:32 PM

Remove unrelated netfront change.

Harbormaster completed remote builds in B19136: Diff 47238.Aug 24 2018, 3:34 PM

alc added a reviewer: markj.Aug 24 2018, 4:09 PM

I'm going on vacations for the whole next week and I'm not taking any computer, so I won't be able to commit this. If someone feels like picking this up in the meantime that's great, if not I will pick it up in two weeks.

royger edited the test plan for this revision. (Show Details)Aug 24 2018, 4:17 PM

kib accepted this revision.Aug 24 2018, 4:46 PM

This revision is now accepted and ready to land.Aug 24 2018, 4:46 PM

markj accepted this revision.Aug 24 2018, 6:14 PM

If this is going to hit 12.0, it must be done before KBI freeze. I will start tinderbox with the change and ask re approval after.

alc added inline comments.Aug 27 2018, 4:28 PM

sys/vm/vm_map.h
214–225	Why not use these functions, rather than a new macro, everywhere the values are being read? And, just use a new macro/function for purposes of initializing the fields.

kib added inline comments.Aug 27 2018, 4:52 PM

sys/vm/vm_map.h
214–225	But then why keeping the macros ? I would go with the functions use as suggested, but change the macros to comment in struct vm_map definition, while directly use header.end and header.start for initialization. Is this fine ?

alc added inline comments.Aug 27 2018, 5:11 PM

sys/vm/vm_map.h
214–225	I'm okay with that plan.

kib commandeered this revision.Aug 27 2018, 5:41 PM

kib edited reviewers, added: royger; removed: kib.

Remove min_offset and max_offset at all.

This revision now requires review to proceed.Aug 27 2018, 5:46 PM

Herald added a subscriber: imp. · View Herald TranscriptAug 27 2018, 5:46 PM

Harbormaster completed remote builds in B19218: Diff 47352.Aug 27 2018, 5:46 PM

kib retitled this revision from mm: rename {max/min}_offset to vm_map_{max/min}_offset to Remove {max/min}_offset, use vm_map_{max/min}..Aug 27 2018, 5:46 PM

kib edited the summary of this revision. (Show Details)

Updated patch passed make tinderbox.

alc accepted this revision.Aug 29 2018, 6:42 AM

alc added inline comments.

sys/vm/vm_map.h
180	"Counterintuitively, the map's min offset value is stored in map->header.end, and its max offset value is stored in map->header.start.

This revision is now accepted and ready to land.Aug 29 2018, 6:42 AM

This revision can now be closed. (Do you have any idea why revisions, like this one, are no longer closing automatically?)

In D16881#361605, @alc wrote:

This revision can now be closed. (Do you have any idea why revisions, like this one, are no longer closing automatically?)

It seems that phabricator did not receive the commit mail, or ignored it.