Skip to content

Conversation

@yamt
Copy link
Contributor

@yamt yamt commented Jan 29, 2020

No description provided.

yamt added 9 commits January 29, 2020 14:31
I guess missing kconfig-conf is a common mistake for
newbies like me.  Bail out earlier instead of producing
random errors during a build.
Namely this changes HOSTOS for macOS from "Other" to "Darwin".

Also, suppress the following harmless messages during a "make":
(Missing redirect in tools/Makefile.host)

	uname: illegal option -- o
	usage: uname [-amnprsv]
Mach-O assembler doesn't have them either.
My copy of GNU ld documentation is saying that -g is ignored.
macOS's ld doesn't know the option and bails out.

	% ld -g
	ld: unknown option: -g
	%
Namely for Mach-O.  Leave __CYGWIN__ case as it is.
The recent x86-64 convention requires 16-byte alignment before
(not after) calling a function.

This fixes snprintf crash I observed on macOS while saving XMM registers.
It seems that "ld -r" on macOS doesn't include objects from
libraries for common symbols. Because of that, sim build
ends up with undefined references to globals like g_binfmts
and g_mmheap.

	@(#)PROGRAM:ld  PROJECT:ld64-530
	BUILD 18:57:17 Dec 13 2019
	configured to support archs: armv6 armv7 armv7s arm64 arm64e arm64_32 i386 x86_64 x86_64h armv6m armv7k armv7m armv7em
	LTO support using: LLVM version 11.0.0, (clang-1100.0.33.17) (static support for 23, runtime is 23)
	TAPI support using: Apple TAPI version 11.0.0 (tapi-1100.0.11)
On some platforms like macOS, we can't create an empty archive.
@yamt
Copy link
Contributor Author

yamt commented Jan 29, 2020

apps part of this: apache/nuttx-apps#35

yamt added 3 commits January 29, 2020 16:21
* ld doesn't have --start-groupi/--end-group things.  As far as I know,
  it works that way by default.

* objcopy with Mach-O support is not widely available.
  (GNU binutils seem to claim the support but it didn't actually work
  for me.  llvm-objcopy --redefine-syms explicitly rejects Mach-O.)
  Instead, use -unexported_symbols_list linker flag to hide symbols
  to avoid conflicts with host symbols.
We usually don't remove input files like $OBJS.
@jerpelea jerpelea merged commit 4174c81 into apache:master Jan 29, 2020
make olddefconfig V=1 || { echo "ERROR: failed to refresh"; exit 1; }
else
make olddefconfig 1>/dev/null 2>&1
make olddefconfig 1>/dev/null || { echo "ERROR: failed to refresh"; exit 1; }
Copy link
Contributor

Choose a reason for hiding this comment

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

need keep 2>&1?

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's intentional to expose the errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok

include ${TOPDIR}/tools/Config.mk

HOSTOS = ${shell uname -o 2>/dev/null || echo "Other"}
HOSTOS = ${shell uname -o 2>/dev/null || uname -s 2>/dev/null || echo "Other"}
Copy link
Contributor

Choose a reason for hiding this comment

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

should we fix the same issue in:
tools/define.sh
tools/incdir.sh
boards/arm/str71x/olimex-strp711/scripts/oocd.sh
arch/x86/src/Makefile

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 guess it doesn't actually matter as they seem just checking if cygwin or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you are right.

@@ -67,6 +67,6 @@
void up_initial_state(struct tcb_s *tcb)
{
memset(&tcb->xcp, 0, sizeof(struct xcptcontext));
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we add a dummy field in xcptcontext instead?

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 don't understand your suggestion. can you explain?

Copy link
Contributor

Choose a reason for hiding this comment

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

Like the below dummy field:
struct xcptcontext
{
void sigdeliver; / Actual type is sig_deliver_t */
xcpt_reg_t dummy;
xcpt_reg_t regs[XCPTCONTEXT_REGS];
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this change is about the actual stack alignment. that is, the value of RSP register.
not about the context structure.

Copy link
Contributor

Choose a reason for hiding this comment

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

My previous comment is wrong, but all places(up_use_stack/up_stack_frame) which modify adj_stack_ptr already align this pointer to 16B boundary, the extra subtraction just make the situation even worse, I can't understand why this change fix your problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the convention is, align before a function call.
that is, from POV of the function being called, it will be aligned after pushing RBP.
note that up_longjmp jumps into the function directly. (without pushing the return address)
RSP at that point should not be aligned.

ifeq ($(HOSTOS),Darwin)
LDSTARTGROUP ?=
LDENDGROUP ?=
LDUNEXPORTSYMBOLS ?= -unexported_symbols_list $(HOSTOS)-names.dat
Copy link
Contributor

Choose a reason for hiding this comment

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

If so, why we don't update other platform to use -unexported_symbols_list? to simplify the logic in Makefile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do gnu ld have the option? my copy of its documentation doesn't have it.

Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 Jan 29, 2020

Choose a reason for hiding this comment

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

Emm, unexported_symbols_list doesn't support on Linux, but after searching we can install binutils and use gobjcopy? even for ld so we don't modify Makefile at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as i mentioned in the commit logs, gnu binutils didn't actually work for me.
objcopy runs without errors, producing a corrupted object.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok.

*/

FAR struct binfmt_s *g_binfmts;
FAR struct binfmt_s *g_binfmts = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

should we use -fno-common option instead? There are many place which don't initialize global variables explicitly.

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 think it works. do you prefer it?

Copy link
Contributor

Choose a reason for hiding this comment

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

There are many places which don't set the global variable to NULL, the same error will report again and again if we turn more option in defconfig. The right fix is to add -fno-common.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, let me try the -fno-common approach later.

#ifdef __CYGWIN__
# define SYMBOL(s) _##s
#else
#ifdef __ELF__
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic is wrong, should it be #ifndef?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, you are right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants